Skip to content

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

Merged
ijuma merged 2 commits intoapache:trunkfrom
ijuma:scala-2.13.11
Jun 20, 2023
Merged

MINOR: Upgrade Scala for Java 20/21 support#13840
ijuma merged 2 commits intoapache:trunkfrom
ijuma:scala-2.13.11

Conversation

@ijuma
Copy link
Copy Markdown
Member

@ijuma ijuma commented Jun 10, 2023 •
edited
Loading

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
Copy Markdown
Member Author

ijuma commented Jun 10, 2023

JDK 8 and 17 builds passed, 11 is queued.

@jlprat
Copy link
Copy Markdown
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
Copy Markdown
Contributor

@jlprat jlprat left a comment •
edited
Loading

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 Kafka Broker label Jun 12, 2023
@jlprat
Copy link
Copy Markdown
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 Kafka Broker labels Jun 12, 2023
jolshan
jolshan reviewed Jun 12, 2023
View reviewed changes
Comment thread core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala
time: Time,
threadNamePrefix: Option[String],
replicationQuotaManager: ReplicationQuotaManager): ReplicaFetcherManager = {
val rm = this
Copy link
Copy Markdown
Member

@jolshan jolshan Jun 12, 2023

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
Copy Markdown
Member Author

@ijuma ijuma Jun 19, 2023

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
Copy Markdown
Member

@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
Copy Markdown
Member 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
Copy Markdown
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
Copy Markdown
Member 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
@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

Development

Successfully merging this pull request may close these issues.

3 participants

@ijuma @jlprat @jolshan