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

Publish pekko based ReactiveMongo binaries #1245

Merged
merged 10 commits into from
Oct 7, 2023

Conversation

SamTheisens
Copy link
Contributor

@SamTheisens SamTheisens commented Aug 6, 2023

Pull Request Checklist

Fixes

Fixes #1244

Purpose

Create a pekko based ReactiveMongo distribution for those who want to adopt Pekko.

Background Context

Implementation details

  • Introduce neutral aliases for Akka actor dependencies to minimize duplication
  • Pulls in akka or pekko dependencies based on a build parameter

Features

  • Publishes pekko variants of all modules
  • Adds pekko based permutations to integration test matrix
  • Does not build scala 3 version for pekko yet, because Pekko needs Scala 3.3.0 or higher

Some integration tests are failing intermittently with timeouts, but it doesn't seem related to the changes.

Related PR on documentation repo ReactiveMongo/reactivemongo-site#316

To minimize duplication:
 - Introduce neutral aliases for Akka actor dependencies
 - Pull in akka or pekko dependencies based on a build parameter
 - Publish pekko variants of all modules
 - Add pekko based permutations to integration test matrix
@SamTheisens SamTheisens force-pushed the feature/pekko-support branch 2 times, most recently from 67ed756 to 72b1a98 Compare August 7, 2023 10:44
Copy link
Member

@cchantep cchantep left a comment

Choose a reason for hiding this comment

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

Thx for the contributions. Few comments.

build.sbt Outdated Show resolved Hide resolved
driver/src/main/scala/api/AsyncDriver.scala Outdated Show resolved Hide resolved
driver/src/main/scala/api/AsyncDriver.scala Outdated Show resolved Hide resolved
driver/src/main/scala/api/Failover.scala Outdated Show resolved Hide resolved
project/Dependencies.scala Show resolved Hide resolved
project/Driver.scala Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
project/Publish.scala Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
project/Common.scala Show resolved Hide resolved
@Fristi
Copy link

Fristi commented Sep 14, 2023

Heya, what's blocking this at the moment? Is there a way I could help?

@cchantep
Copy link
Member

Heya, what's blocking this at the moment? Is there a way I could help?

#1245 (comment)

@SamTheisens
Copy link
Contributor Author

Heya, what's blocking this at the moment? Is there a way I could help?

#1245 (comment)

The compiler crash is the biggest blocker I suppose. I've submitted a crash report scala/scala3#18555.
Unfortunately I haven't been able to construct a minimal reproduction example. That would undoubtedly increase the chances of it being addressed.
@cchantep do you perhaps have some ideas about what might be the root cause?

@Fristi
Copy link

Fristi commented Sep 15, 2023

Would releasing ReactiveMongo with Pekko support and Scala 3.3 support in a seperate PR / version an idea once this is resolved? I would like to move forward with my Akka migration and I'm on a Scala 2.x code base :)

I think most users are using Scala 2.x anyway

@mdedetrich
Copy link

The compiler crash is the biggest blocker I suppose. I've submitted a crash report lampepfl/dotty#18555. Unfortunately I haven't been able to construct a minimal reproduction example. That would undoubtedly increase the chances of it being addressed. @cchantep do you perhaps have some ideas about what might be the root cause?

Some compiler crashes were resolved in 3.3.1, are you still having issues swith this version?

@SamTheisens
Copy link
Contributor Author

The compiler crash is the biggest blocker I suppose. I've submitted a crash report lampepfl/dotty#18555. Unfortunately I haven't been able to construct a minimal reproduction example. That would undoubtedly increase the chances of it being addressed. @cchantep do you perhaps have some ideas about what might be the root cause?

Some compiler crashes were resolved in 3.3.1, are you still having issues swith this version?

I've tried 3.3.1. Unfortunately the issue is still there.

@mdedetrich
Copy link

I've tried 3.3.1. Unfortunately the issue is still there.

Okay thanks, I am cloning the PR as we speak to see if I can diagnose/minimize the issue

@SamTheisens
Copy link
Contributor Author

@cchantep I've found that this change SamTheisens@8cac59b#diff-c6447c7894adc9cfe97724d939b60a87a2aca260563123fefb254d6cf6e73097R17-R18 makes the project compile. Would that be an acceptable solution?

@mdedetrich
Copy link

mdedetrich commented Sep 15, 2023

@cchantep I've found that this change SamTheisens@8cac59b#diff-c6447c7894adc9cfe97724d939b60a87a2aca260563123fefb254d6cf6e73097R17-R18 makes the project compile. Would that be an acceptable solution?

Nice find!

If this is not acceptable, what can be done is you can split out

type Writer[A]
type Reader[A]

into its own trait, and have the scala-2 implementation be

type Writer[A]
type Reader[A]

and scala-3 be

type Writer[A] = BSONDocumentWriter[A]
type Reader[A] = BSONDocumentReader[A]

that way the change is isolated just to Scala 3 artifact.

@cchantep
Copy link
Member

@cchantep I've found that this change SamTheisens@8cac59b#diff-c6447c7894adc9cfe97724d939b60a87a2aca260563123fefb254d6cf6e73097R17-R18 makes the project compile. Would that be an acceptable solution?

Nice find!

If this is not acceptable, what can be done is you can split out

type Writer[A]
type Reader[A]

into its own trait, and have the scala-2 implementation be

type Writer[A]
type Reader[A]

and scala-3 be

type Writer[A] = BSONDocumentWriter[A]
type Reader[A] = BSONDocumentReader[A]

that way the change is isolated just to Scala 3 artifact.

The API won't be changed, for temporary bug on the Scala 3 compiler.
Otherwise then we will have to manage to revert back, with version compatibility complexity.

@mdedetrich
Copy link

mdedetrich commented Sep 15, 2023

Otherwise then we will have to manage to revert back, with version compatibility complexity.

I don't think this change has an impact regarding MiMa because its a type alias, so if correct you can revert it without any implications (its basically adding a default type parameter which would always be overridden anyways).

@cchantep
Copy link
Member

Otherwise then we will have to manage to revert back, with version compatibility complexity.

I don't think this change has an impact regarding MiMa because its a type alias, so if correct you can revert it without any implications (its basically adding a default type parameter which would always be overridden anyways).

Type member are part of the API.
So that's indeed a breaking change.

@mdedetrich
Copy link

Type member are part of the API.
So that's indeed a breaking change.

I understand they are part of the API and it is changing the API, however the term "breaking" has a very explicit technical meaning (talking about bincompat/MiMa here) and I don't think its applicable here.

Undefined type members (which is what type Writer[A]/type Reader[A] are) have to be reified at some point, what type Writer[A] = BSONDocumentWriter[A] is doing is its putting a default value on that type parameter but this shouldn't effect any code that implements this API.

Even if you were to hypothetically add this change and then remove it later, it also should not effect any code implementing this API because because you dealing with an un-implemented type member which at some point has to be evaluated to a proper type.

In fact if you check the issue that caused the change at scala/scala3#16373, at least from what I understand the reason why Scala 3 is complaining in this case is exactly because it can't find a full implementation of that type after erasure (and its not like it can scan all possible implementations of that type outside of the project).

Ill try and do some hacking on the Scala 3 compiler to see if I can avoid this check if we are dealing with undefined type members, but the reason I am bringing this up is that there is a chance that you may need to provide a default value for these undefined type members if a fix doesn't get accepted (I am mentioning this because not every change in the Scala 3 compiler is considered a regression, sometimes if the improvement is warranted then you have to deal with it).

Obviously have to see what Scala 3 team says

@cchantep
Copy link
Member

cchantep commented Sep 15, 2023

I understand they are part of the API and it is changing the API, however the term "breaking" has a very explicit technical meaning (talking about bincompat/MiMa here) and I don't think its applicable here

Please stop arguing, that's not constructive.

From a maintainer point of view, it breaks the API, for an external issue that (should) being fix.

@ReactiveMongo ReactiveMongo locked as too heated and limited conversation to collaborators Sep 15, 2023
because pekko requires scala >= 3.3, which can only be used if and
when scala/scala3#18555 is addressed

branch:
@SamTheisens SamTheisens force-pushed the feature/pekko-support branch 3 times, most recently from d0050ed to 51e65f4 Compare September 26, 2023 14:04
@ReactiveMongo ReactiveMongo unlocked this conversation Oct 1, 2023
@SamTheisens
Copy link
Contributor Author

The scala community is moving away from Akka. Scala 2.13 libraries can be used in Scala 3 projects and it's It's not certain when and if scala/scala3#18555 will be addressed.
Therefore I think it would be great if this PR could be merged and Scala 3 binaries added later in a follow up. As suggested here #1244 (comment). Do you think that would be possible?

project/Common.scala Outdated Show resolved Hide resolved
@cchantep
Copy link
Member

cchantep commented Oct 7, 2023

Thanks

@cchantep cchantep merged commit a506201 into ReactiveMongo:master Oct 7, 2023
1 check passed
@SamTheisens
Copy link
Contributor Author

Thanks

Thanks for merging @cchantep. Out of curiosity: are you planning to make a new 1.1.0-RC12 release with these changes?

@Fristi
Copy link

Fristi commented Oct 9, 2023

I would be keen to see a release with these artifacts been published, a 1.1.0-RC12 would be awesome :)

@SamTheisens
Copy link
Contributor Author

SamTheisens commented Oct 9, 2023

I would be keen to see a release with these artifacts been published, a 1.1.0-RC12 would be awesome :)

@Fristi It's published to sonatype already:

<dependency>
  <groupId>org.reactivemongo</groupId>
  <artifactId>reactivemongo-actors-pekko_2.13</artifactId>
  <version>1.1.0-RC12-SNAPSHOT</version>
</dependency>

@ReactiveMongo ReactiveMongo locked as resolved and limited conversation to collaborators Oct 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pekko support
5 participants