Skip to content
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

jrudolph
Copy link
Member

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.

…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.
And fix bug that remoteAddress parameter was just ignored
@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed validating PR that is currently being validated by Jenkins labels Oct 10, 2017
@akka-ci
Copy link

akka-ci commented Oct 10, 2017

Test PASSed.

Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

@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
Copy link
Member Author

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
@jrudolph jrudolph deleted the jr/w/dont-require-materializer branch October 11, 2017 14:50
@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
Labels
tested PR that was successfully built and tested by Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants