Scala3 migration #11350

jlprat · 2021-09-21T21:09:38Z

This PR lays the groundwork for the Scala3 migration of the code base. Gradle's Scala3 plugin is merged but not released yet (see PR).
During the migration I encountered 2 different bugs in Scala3, they are filed here and here.

Types of changes done in this PR:

Shadowing variables or packages are now errors instead of Scala doing guess work. A fair amount of the changes done in this PR was disambiguation. These include:
- Parameter shadowing
- Shadowing among imports
Parenthesis-less methods called with parameters is not possible any more in Scala3
SockerServer extra classes were split into different files as Scala3 compiler was failing to find them when referenced in other classes in this same file.
foreach java collections method was not directly usable under Scala3, so I transformed it to the Scala collection's one (this might be another bug in Scala3, I need to investigate)
Scala3 fails to determine the right overload method when SAM is involved if types don't match exactly (a.k.a. bug Code not compiling when Unit-returning SAM and overloaded methods involved scala/scala3#13549)
Scala3 companion object of a trait doesn't have the static forwarder methods, needed to reference the companion object "old style" (a.k.a. bug No static forwarder methods in companion of trait scala/scala3#13572)
Extra manual typing was needed occasionally as Scala3 is probably stricter than Scala 2.13

Compiling with Scala3

In order to test this locally one can run the following:

./gradlew wrapper --gradle-distribution-url=https://services.gradle.org/distributions-snapshots/gradle-7.3-20210906222431+0000-bin.zip
And then the usual
./gradlew compileTestScala -PscalaVersion=3.0

Notes

Jackson is using "2.13.0-rc2" version as it's the one that contains Scala3 improvements, it's not really needed to successfully compile though.

Extra information, Scala3 is compiling in "Migration Mode", meaning it outputs some warnings about deprecated and dropped features. See Migration Mode for further info.
All these warnings can be automatically fixed by the Scala compiler itself.

Current Problems

Spotbugs is currently detecting 30 problems with Scala3, it works fine when compiling with Scala 2.13. This currently blocks the execution of core and streams tests. By excluding spotbugs tests can be run and some tests are still failing, I need to find out why is this. Tests run successfully in Scala 2.13. To exclude spotbugs run the following:
./gradlew test -x spotbugsMain -PscalaVersion=3.0

Committer Checklist (excluded from commit message)

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

… missing

jlprat · 2021-09-21T21:12:20Z

Ping @ijuma I was able to have some progress with migrating to Scala3. I still need to check why some test failures occur only on Scala3. Let me know what you think about the changes.

jlprat · 2021-09-21T21:13:06Z

core/src/main/scala/kafka/admin/TopicCommand.scala

@@ -22,7 +22,7 @@ import java.util.{Collections, Properties}
 import joptsimple._
 import kafka.common.AdminCommandFailedException
 import kafka.log.LogConfig
-import kafka.utils._
+import kafka.utils.{immutable => _, _}


Changes like this are due to shadowing between kafka.utils.immutable and the immutable package in scala.collections.

jlprat · 2021-09-21T21:13:40Z

core/src/main/scala/kafka/controller/PartitionStateMachine.scala

@@ -477,7 +477,7 @@ class ZkPartitionStateMachine(config: KafkaConfig,
    } else {
      val (logConfigs, failed) = zkClient.getLogConfigs(
        partitionsWithNoLiveInSyncReplicas.iterator.map { case (partition, _) => partition.topic }.toSet,
-        config.originals()
+        config.originals


Changes like this are the parenthesis-less methods that should be called without parenthesis

jlprat · 2021-09-21T21:14:10Z

core/src/main/scala/kafka/log/LazyIndex.scala

@@ -21,7 +21,7 @@ import java.io.File
 import java.nio.file.{Files, NoSuchFileException}
 import java.util.concurrent.locks.ReentrantLock

-import LazyIndex._
+


This import was causing a cyclic problem in Scala3.

jlprat · 2021-09-21T21:14:45Z

core/src/main/scala/kafka/network/SocketServer.scala

@@ -467,1251 +451,3 @@ object SocketServer {

  val ListenerReconfigurableConfigs = Set(KafkaConfig.MaxConnectionsProp, KafkaConfig.MaxConnectionCreationRateProp)
 }
-


This is what I mentioned about splitting classes present in SocketServer into their own file

jlprat · 2021-09-21T21:15:26Z

core/src/main/scala/kafka/raft/KafkaMetadataLog.scala

@@ -42,11 +42,11 @@ final class KafkaMetadataLog private (
  // Access to this object needs to be synchronized because it is used by the snapshotting thread to notify the
  // polling thread when snapshots are created. This object is also used to store any opened snapshot reader.
  snapshots: mutable.TreeMap[OffsetAndEpoch, Option[FileRawSnapshotReader]],
-  topicPartition: TopicPartition,
+  topicPartitionArg: TopicPartition,


Changes like this are to avoid the shadowing between the parameter and the method

jlprat · 2021-09-21T21:15:51Z

core/src/main/scala/kafka/tools/ConsoleConsumer.scala

@@ -27,7 +27,7 @@ import com.typesafe.scalalogging.LazyLogging
 import joptsimple._
 import kafka.utils.Implicits._
 import kafka.utils.{Exit, _}
-import org.apache.kafka.clients.consumer.{Consumer, ConsumerConfig, ConsumerRecord, KafkaConsumer}
+import org.apache.kafka.clients.consumer.{Consumer, ConsumerConfig => ClientConsumerConfig, ConsumerRecord, KafkaConsumer}


This is done to avoid shadowing

jlprat · 2021-09-21T21:16:32Z

core/src/main/scala/kafka/utils/json/DecodeJson.scala

@@ -85,13 +85,13 @@ object DecodeJson {
    else decodeJson.decodeEither(node).map(Some(_))
  }

-  implicit def decodeSeq[E, S[+T] <: Seq[E]](implicit decodeJson: DecodeJson[E], factory: Factory[E, S[E]]): DecodeJson[S[E]] = (node: JsonNode) => {
+  implicit def decodeSeq[E, S[E] <: Seq[E]](implicit decodeJson: DecodeJson[E], factory: Factory[E, S[E]]): DecodeJson[S[E]] = (node: JsonNode) => {


Scala3 compiler is more strict with type definitions and the previous one wasn't really being satisfied

jlprat · 2021-09-21T21:17:04Z

core/src/test/java/kafka/testkit/KafkaClusterTestKit.java

@@ -234,7 +234,7 @@ public KafkaClusterTestKit build() throws Exception {
                        Option.apply(threadNamePrefix),
                        JavaConverters.asScalaBuffer(Collections.<String>emptyList()).toSeq(),
                        connectFutureManager.future,
-                        Server.SUPPORTED_FEATURES()
+                        Server$.MODULE$.SUPPORTED_FEATURES()


Changes like this are related to scala/scala3#13572

jlprat · 2021-09-21T21:17:34Z

core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala

@@ -2244,7 +2244,7 @@ class AuthorizerIntegrationTest extends BaseRequestTest {
  def testDescribeClusterClusterAuthorizedOperationsWithoutDescribeCluster(): Unit = {
    removeAllClientAcls()

-    for (version <- ApiKeys.DESCRIBE_CLUSTER.oldestVersion to ApiKeys.DESCRIBE_CLUSTER.latestVersion) {
+    for (version <- ApiKeys.DESCRIBE_CLUSTER.oldestVersion.toInt to ApiKeys.DESCRIBE_CLUSTER.latestVersion.toInt) {


The to method is not present in Short type and Scala3 doesn't widen the type automatically

jlprat · 2021-09-21T21:18:08Z

core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala

+      var e = assertThrows(classOf[ExecutionException], () => {
+        alterResult.values.get(topic1).get
+        ()
+      },


This change is because of scala/scala3#13549

jlprat · 2021-09-22T06:05:12Z

One of tus failures was https://issues.apache.org/jira/browse/KAFKA-8785
I need to check the other one

Ignore the others

… instead of the most specific.

Scala3 doesn't convert that freely Int's to Long's anymore

jlprat · 2021-10-08T12:23:25Z

core/src/main/java/kafka/server/builders/LogManagerBuilder.java

@@ -45,7 +46,7 @@
    private long flushStartOffsetCheckpointMs = 10000L;
    private long retentionCheckMs = 1000L;
    private int maxPidExpirationMs = 60000;
-    private ApiVersion interBrokerProtocolVersion = ApiVersion.latestVersion();
+    private ApiVersion interBrokerProtocolVersion = ApiVersion$.MODULE$.latestVersion();


This has been solved in upcoming Scala versions, Bug fix here: scala/scala3#13572

jlprat · 2021-10-08T12:25:11Z

core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala

+                             .ofType(classOf[java.lang.Long])
+                             .defaultsTo(5000L)


This type of changes are due to this bug: scala/scala3#13630

jlprat · 2021-10-08T12:26:03Z

core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala

+        opts.options.valueOf(opts.interBrokerThrottleOpt).longValue(),
+        opts.options.valueOf(opts.replicaAlterLogDirsThrottleOpt).longValue(),
+        opts.options.valueOf(opts.timeoutOpt).longValue())


Some times Scala 3 can't automatically widen Ints to Longs

jlprat · 2021-10-08T12:27:20Z

core/src/test/scala/unit/kafka/coordinator/group/GroupMetadataManagerTest.scala

@@ -2374,7 +2374,7 @@ class GroupMetadataManagerTest {
      minOneMessage = EasyMock.eq(true)))
      .andReturn(FetchDataInfo(LogOffsetMetadata(startOffset), mockRecords))
    EasyMock.expect(replicaManager.getLog(groupMetadataTopicPartition)).andStubReturn(Some(logMock))
-    EasyMock.expect(replicaManager.getLogEndOffset(groupMetadataTopicPartition)).andStubReturn(Some(18))
+    EasyMock.expect(replicaManager.getLogEndOffset(groupMetadataTopicPartition)).andStubReturn(Some(18L))


This is some strange discrepancy in Scala 3 compiler, need to find the cause and report it. Even though the impact on the code base is non-existent

jlprat · 2021-10-08T12:28:10Z

core/src/test/scala/unit/kafka/coordinator/transaction/ProducerIdManagerTest.scala

+  class MockProducerIdManager(val brokerId: Int, val idStart: Long, val idLen: Int)
    extends RPCProducerIdManager(brokerId, () => 1, brokerToController, 100) {

+    var startId = idStart
+    var error: Errors = Errors.NONE
+
    override private[transaction] def sendRequest(): Unit = {
      if (error == Errors.NONE) {
        handleAllocateProducerIdsResponse(new AllocateProducerIdsResponse(
-          new AllocateProducerIdsResponseData().setProducerIdStart(idStart).setProducerIdLen(idLen)))
-        idStart += idLen
+          new AllocateProducerIdsResponseData().setProducerIdStart(startId).setProducerIdLen(idLen)))
+        startId += idLen


These changes are due to this bug that is solved in upcoming Scala versions: scala/scala3#13630

jlprat · 2021-10-08T12:32:15Z

gradle/spotbugs-exclude.xml

+    <Match>
+        <Field name="~.*\$lzy1"/>
+        <Bug pattern="NM_FIELD_NAMING_CONVENTION"/>
+    </Match>


This exception is due to the new naming convention for lazy fields in Scala 3. This is another false positive

jlprat · 2021-10-08T12:32:55Z

gradle/spotbugs-exclude.xml

+    <Match>
+        <!-- This warning only appears during Scala 3 -->
+        <Package name="kafka.server"/>
+        <Source name="AbstractFetcherThread.scala"/>
+        <Bug pattern="RV_RETURN_VALUE_IGNORED_BAD_PRACTICE"/>
+    </Match>
+
+    <Match>
+        <!-- This warning only appears during Scala 3 -->
+        <Package name="kafka.log"/>
+        <Source name="LogLoader.scala"/>
+        <Bug pattern="RV_RETURN_VALUE_IGNORED_BAD_PRACTICE"/>
+    </Match>


These 2 changes seem to me like a false positive when compiling in Scala 3

jlprat · 2021-10-08T12:33:36Z

gradle/spotbugs-exclude.xml

+    <Match>
+        <!-- This warning only appears during Scala 3 -->
+        <Package name="kafka.coordinator.transaction"/>
+        <Source name="ProducerIdManager.scala"/>
+        <Bug pattern="IS2_INCONSISTENT_SYNC"/>
+    </Match>
+
+    <Match>
+        <!-- This warning only appears during Scala 3 -->
+        <Package name="kafka.network"/>
+        <Source name="Acceptor.scala"/>
+        <Bug pattern="IS2_INCONSISTENT_SYNC"/>
+    </Match>
+
+    <Match>
+        <!-- This warning only appears during Scala 3 -->
+        <Package name="kafka.utils"/>
+        <Source name="KafkaScheduler.scala"/>
+        <Bug pattern="IS2_INCONSISTENT_SYNC"/>
+    </Match>
+
+    <Match>
+        <!-- This warning only appears during Scala 3 -->
+        <Package name="kafka.network"/>
+        <Source name="SocketServer.scala"/>
+        <Bug pattern="IS2_INCONSISTENT_SYNC"/>
+    </Match>


This might be a bug in bytecode generation in regards of synchronized blocks. Need to reproduce this and send a bug report.

jlprat · 2021-10-08T12:33:54Z

gradle/spotbugs-exclude.xml

+    <Match>
+        <!-- This warning only appears during Scala 3 -->
+        <Package name="kafka.network"/>
+        <Source name="ConnectionQuotas.scala"/>
+        <Bug pattern="VO_VOLATILE_INCREMENT"/>
+    </Match>


This might be a bug in bytecode generation in regards of volatile variables. Need to reproduce this and send a bug report.

jlprat · 2021-10-08T12:34:50Z

gradle/spotbugs-exclude.xml

+    <Match>
+        <!-- This warning only appears during Scala 3 -->
+        <Package name="kafka.tools"/>
+        <Source name="ConsoleConsumer.scala"/>
+        <Bug pattern="DB_DUPLICATE_BRANCHES"/>
+    </Match>


This seem to me like a false positive when compiling in Scala 3

jlprat · 2021-10-08T12:45:48Z

Current status of the PR:

All tests pass in Scala 3 except one (KafkaApis#getAllTopicMetadataShouldNotCreateTopicOrReturnUnknownTopicPartition) due to this bug with no workaround: Type erased for by-name parameters scala/scala3#13638
All tests pass in Scala 2
It still uses a snapshot build of Gradle

In this PR there are several types of changes:
a) Needed changes because of changes in syntax or compiler being currently more strict
b) Changes where a workaround is present but they are fixed in upcoming Scala versions
c) Changes in the build

I propose perform the Scala 3 migration in several steps:

Firstly apply all changes that would fall into category a)
Once a new Scala version that incorporates the mentioned fixes is released, revisit point changes in point b)
Lastly, once Type erased for by-name parameters scala/scala3#13638 is resolved or a workaround is found and Gradle with Scala 3 support is released we could tackle the final step and incorporate changes mentioned in point c)

What are your thoughts? @ijuma

jvican · 2022-04-05T19:53:10Z

Any plans to prioritize the Scala 3 migration? Would be nice to see this happening.

jlprat · 2022-04-05T20:01:42Z

@jvican some intermediate work is here: #11432

I'd need to resolve s couple if conflicts as the PR got no traction for a while. But I'm happy to do it!

What I think is that migration to Scala 3 might happen when Kafka 4.0.0 is closer in time.

jlprat added 14 commits September 20, 2021 10:27

Scala 3 Compilation with 2.12 support

3533fcc

Add Scala 3.0 to gradlewAll script

72b438f

Test against Gradle nightly with scala3 support

2873540

Some more compilation errors fixed. Implicit conversion for lists are…

ae519e2

… missing

Use latest jackson scala lib

12b45de

Fix Decode for Scala 3

731b527

Bunch of fixes on tests

7b42121

Fix for assertThrows problems

0c43f3e

Workaround for scala/scala3#13572

8aa0690

parentheses-less methods should be called without parens

53cb88b

Workaround for scala/scala3#13549

2cf0742

More instances of scala/scala3#13572

ccacb53

Add missing headers

6d8dc74

fix space

c0ed254

jlprat commented Sep 21, 2021

View reviewed changes

jlprat added 5 commits September 23, 2021 21:37

Fix types in tests

4331f9c

Fix spotbugs complaints about impossible casts.

a2a000c

Ignore the others

Workaround for issue scala/scala3#13630

5b45882

Help type inference. Scala3 now tends to favour the most generic type…

53cd92c

… instead of the most specific.

Explicitly mark number as Long.

06006f5

Scala3 doesn't convert that freely Int's to Long's anymore

jlprat commented Oct 8, 2021

View reviewed changes

jlprat mentioned this pull request Oct 25, 2021

KAFKA-13399 towards scala3 #11432

Closed

3 tasks

francescopellegrini mentioned this pull request Feb 20, 2023

Scala 3 support, pretty please :) embeddedkafka/embedded-kafka#411

Closed

andreisilviudragnea mentioned this pull request Apr 5, 2023

Scala ecosystem status andreisilviudragnea/scala-playground#20

Open

jlprat closed this Jun 1, 2023

		@@ -467,1251 +451,3 @@ object SocketServer {

		val ListenerReconfigurableConfigs = Set(KafkaConfig.MaxConnectionsProp, KafkaConfig.MaxConnectionCreationRateProp)
		}

Scala3 migration #11350

Scala3 migration #11350

Uh oh!

Conversation

jlprat commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Types of changes done in this PR:

Compiling with Scala3

Notes

Current Problems

Committer Checklist (excluded from commit message)

Uh oh!

jlprat commented Sep 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlprat commented Sep 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlprat commented Oct 8, 2021

Uh oh!

jvican commented Apr 5, 2022

Uh oh!

jlprat commented Apr 5, 2022

Uh oh!

Uh oh!

jlprat commented Sep 21, 2021 •

edited

Loading