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

Backport SystemMaterializer API #28494

Merged
merged 6 commits into from Jan 21, 2020
Merged

Conversation

@raboof
Copy link
Member

raboof commented Jan 15, 2020

Without any of the settings changes, not using it anywhere in Akka itself and
without providing the mechanism to create multiple system materializers.

Intended to allow downstream projects to provide API's that require an
ActorSystem instead of a Materializer without requiring Akka 2.6 yet.

Without any of the settings changes, not using it anywhere in Akka itself and
without providing the mechanism to create multiple system materializers.

Intended to allow downstream projects to provide API's that require an
ActorSystem instead of a Materializer without requiring Akka 2.6 yet.
@akka-ci akka-ci added the validating label Jan 15, 2020
Copy link
Member

johanandren left a comment

I wonder if it should include the ClassicActorSystemProvider as well, then we can really introduce methods depending on that which will work for both typed and classic in both 2.5 and 2.6?

akka-stream/src/main/resources/reference.conf Outdated Show resolved Hide resolved
akka-stream/src/main/resources/reference.conf Outdated Show resolved Hide resolved
@akka-ci akka-ci added needs-attention and removed validating labels Jan 15, 2020
@akka-ci

This comment has been minimized.

Copy link

akka-ci commented Jan 15, 2020

Test FAILed.

@akka-ci

This comment has been minimized.

Copy link

akka-ci commented Jan 15, 2020

Test FAILed.

@raboof raboof force-pushed the backport-system-materializer-api branch from dc39092 to c8561a3 Jan 16, 2020
@akka-ci

This comment has been minimized.

Copy link

akka-ci commented Jan 16, 2020

Test PASSed.

Copy link
Member

patriknw left a comment

This was much smaller than I expected. I thought we were also talking about running streams with an (implicit) ActorSystem. Without that it will not solve documentation alignment between 2.5 and 2.6.

Could you give some concrete examples of where we will use this?

Copy link
Member

johanandren left a comment

I'd also expected we need the ClassicActorSystemProvider changes at least, so that for example HTTP can accept one of those and look up the system materializer through it.


"be eagerly started on system startup" in {
system.hasExtension(SystemMaterializer.lookup) should ===(false)
}

This comment has been minimized.

Copy link
@johanandren

johanandren Jan 16, 2020

Member

"not be"

@raboof

This comment has been minimized.

Copy link
Member Author

raboof commented Jan 16, 2020

Could you give some concrete examples of where we will use this?

I'd like to be able to write API's in '2.6 style' (requiring an ActorSystem and no Materializer) in Akka gRPC 1.0.0, where we'll be targeting 2.5 but rather soon move to 1.1.0 which targets 2.6 (while keeping binary compatibility with 1.0.0).

I'd also expected we need the ClassicActorSystemProvider changes at least

Good question! My impression was that ClassicActorSystemProvider was useful for matFromSystem, which would convert an implicit ActorSystem into a Materializer in order to call API methods that accept a Materializer and cannot be changed for binary compatibility reasons.

For Akka gRPC I hoped we can avoid introducing Materializer-based API methods in the first place, and accept ActorSystem (typed or regular) immediately.

Is ClassicActorSystemProvider still needed/useful for new API's?

@johanandren

This comment has been minimized.

Copy link
Member

johanandren commented Jan 16, 2020

ClassicActorSystemProvider will allow the same API for both typed and classic actor system, in addition to allowing use of the system materializer for both.

@raboof

This comment has been minimized.

Copy link
Member Author

raboof commented Jan 16, 2020

ClassicActorSystemProvider will allow the same API for both typed and classic actor system

Right, so instead of:

def myNewApiMethod(implicit system: akka.actor.ActorSystem): ... = ...
def myNewApiMethod(implicit system: akka.actor.typed.ActorSystem): ... = myApiMethod(system.toClassic)

We'd have:

def myNewApiMethod(implicit system: ClassicActorSystemProvider): ... = ...

Is that the only advantage? While the latter saves a line, the extra indirection makes the API more confusing to use. I know for existing API's ClassicActorSystemProvider allowed 2.6 to be much more source-compatible, so there it was worth it, but for new API's I'm not seeing it yet. Am I missing something?

@johanandren

This comment has been minimized.

Copy link
Member

johanandren commented Jan 16, 2020

Another benefit is to not have to depend on Akka typed but still allow having at as possible implicit parameter.

@akka-ci akka-ci added validating tested and removed tested labels Jan 16, 2020
@akka-ci akka-ci removed the validating label Jan 16, 2020
@akka-ci

This comment has been minimized.

Copy link

akka-ci commented Jan 16, 2020

Test PASSed.

But not the new API's that create new system materializers
@raboof

This comment has been minimized.

Copy link
Member Author

raboof commented Jan 16, 2020

Added ClassicActorSystemProvider and the implicit Materializer.matFromSystem (seems not to lead to ambiguous implicits?), but not the new API's that create further system materializers - I don't want to let this grow too much.

@akka-ci

This comment has been minimized.

Copy link

akka-ci commented Jan 16, 2020

Test PASSed.

Copy link
Member

patriknw left a comment

LGTM, good that it's not more

@johanandren johanandren merged commit eff4864 into release-2.5 Jan 21, 2020
3 checks passed
3 checks passed
Jenkins PR Validation Test PASSed. 5355 tests run, 492 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details
@johanandren johanandren deleted the backport-system-materializer-api branch Jan 21, 2020
@johanandren johanandren added this to the 2.5.28 milestone Jan 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.