8000 KAFKA-13980: Upgrade from Scala 2.12.15 to 2.12.17 by mdedetrich · Pull Request #12284 · apache/kafka · GitHub
[go: up one dir, main page]
Skip to content

KAFKA-13980: Upgrade from Scala 2.12.15 to 2.12.17 #12284

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

KAFKA-13980: Upgrade from Scala 2.12.15 to 2.12.17 #12284

wants to merge 1 commit into from

Conversation

mdedetrich
Copy link
Contributor
@mdedetrich mdedetrich commented Jun 11, 2022 •
edited
Loading

This PR updates from Scala 2.12.15 to 2.12.16. Since its a binary compatible change there shouldn't be any problems.

Release notes can be found here https://github.com/scala/scala/releases/tag/v2.12.16

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@mdedetrich mdedetrich force-pushed the update-to-scala-2.12.16 branch from 913fea8 to 74044bd Compare June 13, 2022 22:19
@divijvaidya
Copy link
Member

@mdedetrich could you please re-run the tests (by pushing another commit or by rebasing from trunk & force pushing). It would be ideal if we have a clean test run (with known flaky failures) before we approve this.

@mdedetrich mdedetrich force-pushed the update-to-scala-2.12.16 branch from 74044bd to 4277718 Compare June 15, 2022 09:48
@mdedetrich
Copy link
Contributor Author

@divijvaidya Done

@divijvaidya
Copy link
Member

cc: @ijuma (since you seem have performed scala upgrades in the past)

The test failures do not seem related to this code change to me.

divijvaidya
divijvaidya reviewed Jun 15, 2022
View reviewed changes
@mdedetrich mdedetrich force-pushed the update-to-scala-2.12.16 branch from 4277718 to 4eaa914 Compare June 15, 2022 17:00
divijvaidya
divijvaidya approved these changes Jun 15, 2022
View reviewed changes
Copy link
Member
@divijvaidya divijvaidya left a comment

Choose a reason for hiding this comment

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

LGTM!

@ijuma
Copy link
Member
ijuma commented Jun 15, 2022

I notice there's a regression:

Scala 2.12.16 contains a scala/bug#12605 that was discovered after the artifacts were published. Only mixed compilation of Scala and Java source files together is affected, and only when the Scala code contains references to certain nested classes in the Java sources. The problem manifests as a compile-time type error. Follow link for details and workarounds. We'll fix the problem in Scala 2.12.17 which we expect to release in a few months.

Is there something you're looking for in 2.12.16?

@mdedetrich
Copy link
Contributor Author
mdedetrich commented Jun 15, 2022 •
edited
Loading

Is there something you're looking for in 2.12.16?

So I already had a detailed look into this, it only occurs at compile time (see scala/bug#12605 (comment)) which means that unless you get that specifically mentioned compile error there is no adverse effect. You can see the precise details with the linked ticket at scala/bug#12605 but in summary Kafka is completely unaffected by this (otherwise the scala core would fail to compile).

@ijuma
Copy link
Member
ijuma commented Jun 16, 2022

@mdedetrich Yes, kafka is unaffacted by it now, but it could be affected in the future. If there's a clear benefit for going with 2.12.16, we can go ahead though. Is there?

@mdedetrich
Copy link
Contributor Author
mdedetrich commented Jun 17, 2022 •
edited
Loading

Yes, kafka is unaffacted by it now, but it could be affected in the future.

The fix for this has already been merged into Scala and it will be included in the 2.12.17 release which is being expedited. So while its theoritically possible Kafka could be effected by this change if you do write very specific Scala/Java code that hits this bug, i.e.

public class C {
  public class I { public class J {} }
  public void t(I.J j) {}
}

Its highly unlikely this would happen in Kafka before Scala 2.12.17 is released (current eta 2-3 months) and the appropriate Scala version bump is put into Kafka.

Even then it may be possible that Kafka is completely unaffected by this issue because it only occurs if the build tool (in our case Gradle) compiles Java/Scala at the same time. If you compile Scala and then Java afterwards you are completely unaffected. If necessary I can see if this is the case.

If there's a clear benefit for going with 2.12.16, we can go ahead though. Is there?

So according to the release notes this release of Scala mainly addressed JDK related issues, i.e.

This release improves compatibility with recent JDKs:

Use ASM 9.3, enabling JDK 19 support (scala/scala#10000)
Make -target support JDK 8 through 19 (and deprecate 5 through 7) (scala/scala#9916)
Fix codegen for MethodHandle.invoke (et al) under JDK 17 -release (scala/scala#9930)
Deprecate AnyVal#formatted(formatString), to avoid conflict with JDK 15+ method (scala/scala#9783)

So I would say there is clear benefit for people that happen to be using newer JDK versions.

@mdedetrich mdedetrich force-pushed the update-to-scala-2.12.16 branch from 4eaa914 to b070d08 Compare July 27, 2022 12:46
@mimaison
Copy link
Member

Can you also update bin/kafka-run-class.sh and bin/windows/kafka-run-class.bat?

@mdedetrich
Copy link
Contributor Author

@mimaison

Can you also update bin/kafka-run-class.sh and bin/windows/kafka-run-class.bat?

I just checked those files and it seems that this is not necessary because we are not updating the latest Scala series but the one behind it. Kafka supports Scala 2.13.x and 2.12.x with 2.13.x being the default which is the version being referenced in bin/kafka-run-class.sh/bin/windows/kafka-run-class.bat. This PR however only updates Scala 2.12.x (specifically 2.12.5 to 2.12.16)

@mimaison
Copy link
Member

Ah yes, you're right! My bad

@mdedetrich
Copy link
Contributor Author

@ijuma @mimaison Any chance of a review?

@mdedetrich mdedetrich force-pushed the update-to-scala-2.12.16 branch from b070d08 to b279b84 Compare September 17, 2022 12:29
@mdedetrich
Copy link
Contributor Author
mdedetrich commented Sep 17, 2022 •
edited
Loading

Scala 2.12.17 just came out so I have rebased the PR with the new Scala version. This also means that all of the qualms mentioned in #12284 (comment) aren't relevant anymore.

Release details can be found here https://github.com/scala/scala/releases/tag/v2.12.17

@mdedetrich mdedetrich changed the title KAFKA-13980: Upgrade from Scala 2.12.15 to 2.12.16 KAFKA-13980: Upgrade from Scala 2.12.15 to 2.12.17 Sep 19, 2022
@mimaison
Copy link
Member
mimaison commented Feb 2, 2024

We upgrade to Scala 2.12.18 in dfaae31. Closing this PR.

@mimaison mimaison closed this Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ijuma ijuma ijuma left review comments

@divijvaidya divijvaidya divijvaidya approved these changes

@Kvicii Kvicii Kvicii approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mdedetrich @divijvaidya @ijuma @mimaison @Kvicii
0