search

apache / pekko-http

The Streaming-first HTTP server/module of Apache Pekko
https://pekko.apache.org/
Apache License 2.0
152 stars 36 forks source link

Replace `statefulMapConcat` with `statefulMap` + `mapConcat` in codebase #395

Closed mdedetrich closed 2 months ago

mdedetrich commented 8 months ago

As the title suggests, statefulMapConcat is deprecated so it should replaced with statefulMap + mapConcat so that the 1.1.0 version of pekko-http isn't relying on deprecated pekko methods.

laglangyue commented 8 months ago

also found in pekko-connectors

mdedetrich commented 8 months ago

also found in pekko-connectors

Thanks, issue created at https://github.com/apache/incubator-pekko-connectors/issues/307

pjfanning commented 8 months ago

https://github.com/apache/incubator-pekko-http/pull/311 was merged already but a quick search indicates that we might still have 1 or 2 remaining uses.

He-Pin commented 8 months ago

I did a check seems there are no usage now. @pjfanning , the only lefts are in scaladoc and benchmarks

jrudolph commented 7 months ago

This might have performance implications (mapConcat had performance issues in the past) so make sure to benchmark before doing these changes.

pjfanning commented 7 months ago

This was deprecated in https://github.com/apache/incubator-pekko/issues/601 - if we think deprecating the code was a mistake then we can reopen that discussion. If we think it was the right decision to deprecate statefulMapConcat, then we should start removing its usage.

jrudolph commented 7 months ago

I like the change from an API orthogonality perspective, just saying that it might not come for free. In the worst case, it could be possible to keep a copy in StreamUtils, but better if that is not needed.

He-Pin commented 7 months ago

I think this issue can be closed as the associated change has been one. in https://github.com/apache/incubator-pekko-http/pull/311.

Or we can do some dedicated implementation just for pekko-http usage in StreamUtils, without any additional allocations.

He-Pin commented 7 months ago

The current statefulMap or mapAccumlate implementation will need a (S, A) , which is a kind of allocation, in JEP https://openjdk.org/jeps/461 @viktorklang @viktorklang-ora is using an object which reducing this allocation.

He-Pin commented 7 months ago

@jrudolph I will review the changes and try to come up with an allocation-free implementation in pekko-http.

pjfanning commented 2 months ago

I'm going to close this because we undeprecated statefulMapConcat. Feel free to reopen.