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.

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

MINOR: Upgrade Scala for Java 20/21 support #13840

Merged
merged 2 commits into from Jun 20, 2023
Merged

MINOR: Upgrade Scala for Java 20/21 support #13840

merged 2 commits into from Jun 20, 2023

Conversation

ijuma
Copy link
Contributor

@ijuma ijuma commented Jun 10, 2023 •
edited

Upgrade to Scala 2.13.11 and Scala 2.12.18. A minor test change was required
to fix compilation with Scala 2.13.11.

Scala 2.13 release notes:

Scala 2.12 release notes:

Committer Checklist (excluded from commit message)

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

@ijuma ijuma requested a review from jolshan June 10, 2023 17:38
@ijuma
Copy link
Contributor Author

ijuma commented Jun 10, 2023

JDK 8 and 17 builds passed, 11 is queued.

@jlprat
Copy link
Contributor

jlprat commented Jun 12, 2023

Hi @ijuma from the release notes:

Known issues

A few regressions have been discovered:

Exhaustivity checker emits spurious warning when matching on Java enum type (2.13.11 regression) (scala/bug#12800)
Duplicated @Deprecated annotations when extending Java interface with deprecated default method cause java.lang.annotation.AnnotationFormatError when accessed via Java reflection (2.13.11 regression) (scala/bug#12799)

We'll address these in Scala 2.13.12.

I don't think we are hitting those, but I just wanted to make sure we are aware of these before merging.

jlprat
jlprat approved these changes Jun 12, 2023
View reviewed changes
Copy link
Contributor

@jlprat jlprat left a comment •
edited

Choose a reason for hiding this comment

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

Hi @ijuma thanks for this patch.
I found a shorter way to make the test compile (and run successfully). One needs to type the replicaManager variable in line 2481:

val replicaManager: ReplicaManager = new ReplicaManager(

But anyway, your patch LGTM 👍 (given the tests complete, it seems JDK 11 and Scala 2.13 got aborted)

@jlprat jlprat added the core label Jun 12, 2023
@jlprat
Copy link
Contributor

jlprat commented Jun 12, 2023

I triggered the build again

@jlprat jlprat added dependencies Pull requests that update a dependency file and removed core labels Jun 12, 2023
jolshan
jolshan reviewed Jun 12, 2023
View reviewed changes
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala
@@ -2499,15 +2499,16 @@ class ReplicaManagerTest {
time: Time,
threadNamePrefix: Option[String],
replicationQuotaManager: ReplicationQuotaManager): ReplicaFetcherManager = {
val rm = this
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to replace "this" in the line below with rm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't make a difference since it's before the scope of the inner class of ReplicaFetcherManager is created, but I made this change for clarity.

jolshan
jolshan approved these changes Jun 12, 2023
View reviewed changes
Copy link
Contributor

@jolshan jolshan left a comment

Choose a reason for hiding this comment

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

left one small comment. Tests seem to be looking better.

@ijuma
Copy link
Contributor Author

ijuma commented Jun 19, 2023

Thanks for the reviews. A couple of responses.

I don't think we are hitting those, but I just wanted to make sure we are aware of these before merging.

Yes, I saw that note but didn't seem like a blocker for us.

I found a shorter way to make the test compile (and run successfully). One needs to type the replicaManager variable in line 2481:

Interesting - I tried that originally and it didn't work for me. In any case, the fact that we were referring to this inside a new anonymous class to refer to the outer ReplicaManager anonymous class is confusing. So, I think the change I made makes things clearer in the end.

@jlprat
Copy link
Contributor

jlprat commented Jun 19, 2023

So, I think the change I made makes things clearer in the end.

I agree. Thanks @ijuma

@ijuma
Copy link
Contributor Author

ijuma commented Jun 20, 2023

JDK 11 build passed, there were two unrelated failures outside of that:

Build / JDK 17 and Scala 2.13 / testMaxConnectionsPerIp() – kafka.network.SocketServerTest
15s
Build / JDK 8 and Scala 2.12 / testBalancePartitionLeaders() – org.apache.kafka.controller.QuorumControllerTest

@ijuma ijuma merged commit dfaae31 into apache:trunk Jun 20, 2023
1 check failed
@ijuma ijuma deleted the scala-2.13.11 branch June 20, 2023 17:29
udaynpusa pushed a commit to mapr/kafka that referenced this pull request Apr 25, 2024
@ijuma @ychernysh
KAFKA-1001 Update scala version to resolve CVE-2022-36944
3c6ef14 
This change bumps scala version to 2.13.12 and cherry-picks squashed combination of the following commits (only scala-related changes):
dfaae31 MINOR: Upgrade Scala for Java 20/21 support (apache#13840)
6ae08c4 KAFKA-14256: Upgrade from Scala 2.13.8 to 2.13.10 (apache#12675)
7c2d672 MINOR: Update library dependencies (Q1 2022) (apache#11306)
a8bd649 MINOR: Update Scala to 2.13.6 (apache#10711)
dd34e40 MINOR: Update Scala to 2.13.5 (apache#10169)
cbf8ad2 MINOR: Upgrade to Scala 2.13.4 (apache#9643)
7f90a58 MINOR: Update Scala to 2.13.3 (apache#8931)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlprat jlprat jlprat approved these changes

@jolshan jolshan jolshan approved these changes

Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
No milestone
3 participants
@ijuma @jlprat @jolshan