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

scala3 support for slick #223

Merged
merged 16 commits into from
Mar 7, 2024
Merged

scala3 support for slick #223

merged 16 commits into from
Mar 7, 2024

Conversation

pjfanning
Copy link
Contributor

@pjfanning pjfanning commented Aug 14, 2023

@mdedetrich
Copy link
Contributor

So the slick tests are definitely flaky, I added a commit which seems to have helped (i.e. only failing ~30% of the time). I don't recall if the Slick tests were normally this flaky (don't remember this being the case) and it only seems to be happening on Scala 3.

@pjfanning
Copy link
Contributor Author

The Slick tests haven't been all that flaky up until now.

@mdedetrich
Copy link
Contributor

mdedetrich commented Aug 24, 2023

The Slick tests haven't been all that flaky up until now.

Yes this is my recollection as well, the flakiness is also only occurring with Slick Scala 3

@mdedetrich
Copy link
Contributor

mdedetrich commented Nov 22, 2023

@pjfanning There are MiMa related failures, although tbh we never agreed for MiMa/SemVer on this pekko module and Slick 3.5 is slated for 1.1.x anyways

@pjfanning pjfanning added this to the 1.1.0 milestone Dec 23, 2023
@mdedetrich mdedetrich force-pushed the scala3-slick branch 4 times, most recently from 0abbbab to b41fad0 Compare February 4, 2024 03:13
@mdedetrich
Copy link
Contributor

@pjfanning I just cleaned up the branch which included some rebases so I would just pull the latest changes ignoring the local ones.

Good news is that Slick 3.5.0-M4 seems to work fine with none of the previous problems that we were experiencing.

@mdedetrich mdedetrich marked this pull request as ready for review February 22, 2024 06:22
@mdedetrich
Copy link
Contributor

mdedetrich commented Feb 22, 2024

So it is being asked if we can publish these changes so that people can test Slick 3.5.0-RC1 before Slick does a full release. Since 1.0.x branch is already setup if we want to do snapshots we can just merge this PR as is, milestone would obviously require a proper vote which I am not against and given what we discussed at #443 (comment) creating a milestone right now (aside from just Slick 3.5.0-RC1) may not be a bad idea since we already did quite a few major dependency updates/changes.

@pjfanning @nafg wdyt?

@mdedetrich
Copy link
Contributor

Please don't merge this PR until we make a discussion about whether its appropriate for us as a snapshot

@raboof
Copy link
Member

raboof commented Feb 22, 2024

Ideally I think releases should not depend on snapshot/RC dependencies, and the main branch should always be in a release-able state. I guess we can deviate from that if there is a strong reason for it. Alternatively, would it be possible to set up our GitHub Actions so that we can publish a snapshot built off a branch/PR?

@pjfanning pjfanning marked this pull request as draft February 22, 2024 08:55
@pjfanning
Copy link
Contributor Author

I agree that it is too time consuming for us to do releases to test non-ASF jars. If someone really wanted to test this, they could build pekko-connectors-slick jars themselves by doing a git checkout of this PR's branch and running sbt commands.

@mdedetrich
Copy link
Contributor

Replied at slick/slick#2891 (reply in thread)

@pjfanning pjfanning marked this pull request as ready for review March 7, 2024 11:39
@@ -54,6 +51,7 @@ object Slick {
session: SlickSession,
query: String,
mapper: JFunction[SlickRow, T]): Source[T, NotUsed] = {

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Random new line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just spreading out the code to make it more readable

Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

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

Just one change I would like to explore

slick/src/test/java/docs/javadsl/SlickTest.java Outdated Show resolved Hide resolved
Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

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

lgtm, CI still passes even with test timeouts reverted

@pjfanning pjfanning merged commit 0450443 into apache:main Mar 7, 2024
50 checks passed
@pjfanning pjfanning deleted the scala3-slick branch March 7, 2024 15:25
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.

None yet

3 participants