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

Scala 3 support #775

Merged
merged 17 commits into from
Mar 8, 2024
Merged

Scala 3 support #775

merged 17 commits into from
Mar 8, 2024

Conversation

jtjeferreira
Copy link
Contributor

Adds scala 3 support

@lightbend-cla-validator

Hi @jtjeferreira,

Thank you for your contribution! We really value the time you've taken to put this together.

We see that you have signed the Lightbend Contributors License Agreement before, however, the CLA has changed since you last signed it.
Please review the new CLA and sign it before we proceed with reviewing this pull request:

http://www.lightbend.com/contribute/cla

Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

We'll need a non-milestone Slick release before we can merge this to main. Good to have this prepared once that arrives though! 👍

@johanandren
Copy link
Member

Hey @jtjeferreira any chance you have time to rebase on master to solve the conflict so we can see if Slick 3.5.0 RC1 works?

@jtjeferreira
Copy link
Contributor Author

Hey @jtjeferreira any chance you have time to rebase on master to solve the conflict so we can see if Slick 3.5.0 RC1 works?

I tried the "Resolve Conflicts" from the GitHub UI on my phone and the conflict seems simple. I will have a look tomorrow...

@jtjeferreira
Copy link
Contributor Author

Hey @jtjeferreira any chance you have time to rebase on master to solve the conflict so we can see if Slick 3.5.0 RC1 works?

I tried the "Resolve Conflicts" from the GitHub UI on my phone and the conflict seems simple. I will have a look tomorrow...

I fixed the conflicts. Can you approve the workflows?

PS: There is a test that will fail, due to a compiler bug

[info] H2ScalaEventsByTagMigrationTest:
[info] - should migrate event tag to new way *** FAILED *** (2 milliseconds)
[info]   java.lang.ClassCastException: class com.typesafe.config.impl.SimpleConfig cannot be cast to class java.lang.String (com.typesafe.config.impl.SimpleConfig is in unnamed module of loader sbt.internal.LayeredClassLoader @611cd810; java.lang.String is in module java.base of loader 'bootstrap')
[info]   at akka.persistence.jdbc.query.EventsByTagMigrationTest.config$accessor(EventsByTagMigrationTest.scala:31)
[info]   at akka.persistence.jdbc.query.EventsByTagMigrationTest.withRollingUpdateActorSystem(EventsByTagMigrationTest.scala:161)
[info]   at akka.persistence.jdbc.query.EventsByTagMigrationTest.testFun$proxy1$1(EventsByTagMigrationTest.scala:188)
[info]   at akka.persistence.jdbc.query.EventsByTagMigrationTest.$init$$$anonfun$1(EventsByTagMigrationTest.scala:171)

I will push the fix after so we can have proof of the bug (and the bugfix)

@jtjeferreira
Copy link
Contributor Author

Thanks for approving the workflows. There are 2 issues:

  • I haven't changed the workflows to use scala3. Any tip where I should do that? .github/workflows/checks.yml? Any examples from other akka projects I could follow?
  • Mima warnings in 2.13 on JdbcBackend#DatabaseDef vs JdbcBackend#JdbcDatabaseDef which changed in slick 3.5.0. Can I add a mima filter?

@johanandren
Copy link
Member

I pushed a commit adding Test/compile on Scala 3 to CI PR validation now. There are some related failures to look into - methods that end up with the same signature after erasure.

Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

Looks good so far! Some smaller things and the conflicting overloads and this is as ready as it gets until slick 3.5.0 is released 👍


ProblemFilters.exclude[IncompatibleResultTypeProblem]("akka.persistence.jdbc.db.SlickDatabase.forConfig")
ProblemFilters.exclude[IncompatibleResultTypeProblem]("akka.persistence.jdbc.db.SlickDatabase.database")
ProblemFilters.exclude[ReversedMissingMethodProblem]("akka.persistence.jdbc.db.SlickDatabase.database")
Copy link
Member

Choose a reason for hiding this comment

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

SlickDatabase already is internal, so this is fine

ProblemFilters.exclude[IncompatibleMethTypeProblem]("akka.persistence.jdbc.snapshot.dao.DefaultSnapshotDao.this")
ProblemFilters.exclude[IncompatibleMethTypeProblem]("akka.persistence.jdbc.snapshot.dao.legacy.ByteArraySnapshotDao.this")
ProblemFilters.exclude[IncompatibleResultTypeProblem]("akka.persistence.jdbc.state.JdbcDurableStateStoreProvider.db")
ProblemFilters.exclude[IncompatibleMethTypeProblem]("akka.persistence.jdbc.state.scaladsl.JdbcDurableStateStore.this")
Copy link
Member

Choose a reason for hiding this comment

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

Both fine to exclude:

Constructor isn't used by user code (instantiated by akka-persistence). A bit curious that it shows up, I can't see that it wasn't touched for any of the listed types though.

db field shouldn't ever be touched by user code, effectively protected.


override val javadslReadJournal = new javadsl.JdbcReadJournal(scaladslReadJournal)
override def javadslReadJournal(): javadsl.JdbcReadJournal = new javadsl.JdbcReadJournal(scaladslReadJournal())
Copy link
Member

Choose a reason for hiding this comment

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

The returned instances are cached and shared by akka-persistence internals so this change is fine. 👍

def addRange(from: OrderingId, until: OrderingId): MissingElements = {
val newRange = from.until(until)
MissingElements(elements :+ newRange)
}
def contains(id: OrderingId): Boolean = elements.exists(_.containsTyped(id))
def isEmpty: Boolean = elements.forall(_.isEmpty)
}
private object MissingElements {
object MissingElements {
Copy link
Member

Choose a reason for hiding this comment

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

Make the multi-param JournalSequenceActor.receive(currentMaxOrdering ...) private instead of changing these (it's an internal factory for Receive instances, not called from the outside)

@@ -46,7 +46,7 @@ import akka.annotation.InternalApi
* Efficient representation of missing elements using NumericRanges.
* It can be seen as a collection of GlobalOffset
*/
private case class MissingElements(elements: Seq[NumericRange[GlobalOffset]]) {
case class MissingElements(elements: Seq[NumericRange[GlobalOffset]]) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here, make the method private instead

@jtjeferreira
Copy link
Contributor Author

I pushed a commit adding Test/compile on Scala 3 to CI PR validation now.

Thanks

There are some related failures to look into - methods that end up with the same signature after erasure.

I have seen that error locally. If I comment the @ApiMayChange it compiles. If I add it again and compile incrementally it works... I guess this is another compiler bug?

@johanandren
Copy link
Member

If I comment the @ApiMayChange it compiles. If I add it again and compile incrementally it works... I guess this is another compiler bug?

Sounds like that, removing that annotation and adding Scaladoc saying it "API may change" is fine IMO

@jtjeferreira
Copy link
Contributor Author

If I comment the @ApiMayChange it compiles. If I add it again and compile incrementally it works... I guess this is another compiler bug?

Sounds like that, removing that annotation and adding Scaladoc saying it "API may change" is fine IMO

Meanwhile, I found a workaround (see 45bbe4a).

I think I addressed all your feedback and also fixed mima after e6422a4

@johanandren
Copy link
Member

Even better 👍

…ing persisted when the query is executed"

increase toStrict timeout
@jtjeferreira
Copy link
Contributor Author

Regarding the failure in H2ScalaCurrentEventsByTagTest#should complete without any gaps in case events are being persisted when the query is executed I was also facing the problem locally, but I was hoping it was due to some slowness in my machine.

I fixed it with 7199f96, but I wonder if this is a problem with slick 3.5.0...

Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the good work @jtjeferreira !

@johanandren johanandren merged commit df901fa into akka:master Mar 8, 2024
14 checks passed
@johanandren johanandren modified the milestones: 5.3.1, 5.4.0 Mar 8, 2024
@jtjeferreira
Copy link
Contributor Author

LGTM, thanks for the good work @jtjeferreira !

Thanks. Looking forward for a release

@jtjeferreira
Copy link
Contributor Author

Looks like there is a problem in docs https://github.com/akka/akka-persistence-jdbc/actions/runs/8201918926/job/22431619062:

[error] missing argument for option -skip-packages

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants