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

actor: allow seamless access to untyped extensions given typed ActorSystem #28294

Merged

Conversation

@jrudolph
Copy link
Member

jrudolph commented Dec 2, 2019

Another try after #27760 to solve that problem.

@jrudolph jrudolph requested review from patriknw and johanandren and removed request for patriknw Dec 2, 2019
Copy link
Member

johanandren left a comment

This looks awesome, almost too good to be true.

* }}}
*
*/
def get(system: ClassicActorSystemProvider): T = apply(system)

This comment has been minimized.

Copy link
@johanandren

johanandren Dec 2, 2019

Member

I don't get how this is not an ambiguous overload in at Java, but I see below that you are actually calling it from Java code...

This comment has been minimized.

Copy link
@jrudolph

jrudolph Dec 2, 2019

Author Member

For typed ActorSystems only this one matches. For untyped ActorSystems both would match but the old one would be more specific and chosen for that reason.

@akka-ci akka-ci added tested and removed validating labels Dec 2, 2019
@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Dec 2, 2019

Test PASSed.

@jrudolph

This comment has been minimized.

Copy link
Member Author

jrudolph commented Dec 2, 2019

Interesting mima failure for 2.12. Will look into it tomorrow. It seems like adding an overload fixed the forwarders so you wouldn't have to overload the get from Java any more? That's weird.

Copy link
Member

patriknw left a comment

Excellent if this works. Do we have enough Java compile coverage from Typed?

/**
* Returns an instance of the extension identified by this ExtensionId instance.
*/
def apply(system: ClassicActorSystemProvider): T = apply(system.classicSystem)

This comment has been minimized.

Copy link
@patriknw

patriknw Dec 3, 2019

Member

So the difference from what I tried in #27760 (comment) is that it's defined in the ExtensionId instead of the concrete class. Great.

@jrudolph

This comment has been minimized.

Copy link
Member Author

jrudolph commented Dec 3, 2019

Interesting mima failure for 2.12.

It seems Mima complains because it checks against older versions of Akka 2.5.x that used a Scala version that generated more static forwarders than required (i.e. both for the generic signature and for the overridden with concrete return type one). Now Mima gets confused when we add even more overloads because it seems to wrongly match those static forwarders.

@jrudolph

This comment has been minimized.

Copy link
Member Author

jrudolph commented Dec 3, 2019

Excellent if this works. Do we have enough Java compile coverage from Typed?

We have coverage in the docs tests.

@jrudolph

This comment has been minimized.

Copy link
Member Author

jrudolph commented Dec 3, 2019

Added Mima exclusions.

@akka-ci akka-ci added validating and removed tested labels Dec 3, 2019
@jrudolph

This comment has been minimized.

Copy link
Member Author

jrudolph commented Dec 3, 2019

Added Mima exclusions.

Seems I added them to the wrong folder? They should go to 2.6.0 mima-filters...

@jrudolph

This comment has been minimized.

Copy link
Member Author

jrudolph commented Dec 3, 2019

Seems I added them to the wrong folder? They should go to 2.6.0 mima-filters...

And why? Because our mima check is currently broken to check against 2.6.0 for all non-typed modules. PR upcoming

@akka-ci akka-ci added tested and removed validating labels Dec 3, 2019
@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Dec 3, 2019

Test PASSed.

1 similar comment
@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Dec 3, 2019

Test PASSed.

Copy link
Member

patriknw left a comment

LGTM

@@ -0,0 +1,12 @@
# Incompatibilities against Akka < 2.5.17 where extra static bridge methods were generated that

This comment has been minimized.

Copy link
@patriknw

patriknw Dec 3, 2019

Member

previously we added filters to 2.5.x.backwards.excludes, will this work when we release 2.5.27?

This comment has been minimized.

Copy link
@raboof

raboof Dec 5, 2019

Member

IIRC 2.5.x.backwards.excludes was only needed when we were making changes on master that would conflict with 'the latest 2.5.x' but hadn't released 2.6.0 yet. Nowadays adding them to 2.6.0.backwards.excludes should be sufficient. In this particular case adding them to 2.5.16.backwards.excludes might even be enough, since apparently mima doesn't get confused about akka versions >= 2.5.17? I'll try that.

This comment has been minimized.

Copy link
@jrudolph

jrudolph Dec 5, 2019

Author Member

I think they can just go to 2.6.0. I had it already fixed but forgot to push, sorry.

@jrudolph

This comment has been minimized.

Copy link
Member Author

jrudolph commented Dec 3, 2019

jrudolph added 4 commits Dec 2, 2019
…ystem
jrudolph added 3 commits Dec 3, 2019
@jrudolph jrudolph force-pushed the jrudolph:27760-easier-extension-access-from-typed branch from a47b28b to ad5e4c0 Dec 5, 2019
@jrudolph

This comment has been minimized.

Copy link
Member Author

jrudolph commented Dec 5, 2019

A few mima excludes were still missing, I added them now and put all of those into the right directory.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Dec 5, 2019

Test PASSed.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Dec 5, 2019

Test PASSed.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Dec 5, 2019

Test PASSed.

@patriknw

This comment has been minimized.

Copy link
Member

patriknw commented Dec 5, 2019

build fail is #28312

Copy link
Member

johanandren left a comment

LGTM, nice!

@raboof
raboof approved these changes Dec 5, 2019
@raboof raboof merged commit 702b6a7 into akka:master Dec 5, 2019
3 checks passed
3 checks passed
Jenkins PR Validation Test PASSed. 8601 tests run, 478 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
@patriknw patriknw mentioned this pull request Dec 6, 2019
@raboof raboof added this to the 2.6.1 milestone Dec 6, 2019
navaro1 added a commit to navaro1/akka that referenced this pull request Dec 17, 2019
…ystem (akka#28294)

* actor: allow seamless access to untyped extensions given typed ActorSystem

* add overrides with concrete type for Java API everywhere
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.