New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In top-level API don't require materializer where it's just ignored afterwards #1464

Merged
merged 4 commits into from Oct 11, 2017

Conversation

Projects
None yet
3 participants
@jrudolph
Member

jrudolph commented Oct 10, 2017

It seems we had lots of these left over I guess from the early days where creating substreams required materializers to be passed. It seems no one cleaned that up so far. The changes were somewhat big to keep binary compatibility at least for a while. In Scala, it's binary and source compatible (if the materializer was passed implicitly). Java users need to remove the materializer argument.

Going forward, if we deprecate the old methods now and release another 10.0.x in October, we might think about cleaning those methods up for 10.1.0.

jrudolph added some commits Oct 9, 2017

!htc remove redundant implicit Materializer parameter on Http.singleR…
…equest

Requires a bit of an overload dance to ensure keeping binary compatibility.

In Scala, also source compatibility is kept. On the Java side, the materializer
arguments must be removed from invocations.
!htc remove implicit materializer from serverLayer methods
And fix bug that remoteAddress parameter was just ignored

@akka-ci akka-ci added validating tested and removed validating labels Oct 10, 2017

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Oct 10, 2017

Collaborator

Test PASSed.

Collaborator

akka-ci commented Oct 10, 2017

Test PASSed.

@raboof

raboof approved these changes Oct 10, 2017

Definitely makes things a bit nicer. Hard to say for me what the chance is that we'd discover later that it would actually be nice to have access to a materializer in those places in the future - but probably low since this implementation works well and is now fairly mature.

@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Oct 11, 2017

Member

@raboof yes, that's something I also thought about. For all of the shared pool APIs it just doesn't make sense to pass a materializer because it would just be unclear what it would mean if different invocations targeting the same shared pool would get different materializers.

For serverLayer, we can hope that any change that would require a materializer can be implemented by using a subFusingMaterializer.

Member

jrudolph commented Oct 11, 2017

@raboof yes, that's something I also thought about. For all of the shared pool APIs it just doesn't make sense to pass a materializer because it would just be unclear what it would mean if different invocations targeting the same shared pool would get different materializers.

For serverLayer, we can hope that any change that would require a materializer can be implemented by using a subFusingMaterializer.

@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Oct 11, 2017

Member

Btw the reason I looked into this was that I saw code somewhere with multiple ActorMaterializers calling those shared pool methods and I wondered what would happen.

Member

jrudolph commented Oct 11, 2017

Btw the reason I looked into this was that I saw code somewhere with multiple ActorMaterializers calling those shared pool methods and I wondered what would happen.

@jrudolph jrudolph merged commit cf06b03 into akka:master Oct 11, 2017

3 checks passed

Jenkins PR Validation Test PASSed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@jrudolph jrudolph deleted the jrudolph:jr/w/dont-require-materializer branch Oct 11, 2017

@jrudolph jrudolph added this to the 10.0.11 milestone Oct 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment