KAFKA-9324: Drop support for Scala 2.11 (KIP-531) #7859

ijuma · 2019-12-22T05:17:08Z

Adjust build and documentation.
Use lambda syntax for SAM types in core, streams-scala and
connect-runtime modules.
Remove runnable and newThread from CoreUtils as lambda
syntax for SAM types make them unnecessary.
Remove stale comment in FunctionsCompatConversions,
KGroupedStream, KGroupedTable' and KStream` about Scala 2.11,
the conversions are needed for Scala 2.12 too.
Deprecate org.apache.kafka.streams.scala.kstream.Suppressed
and use org.apache.kafka.streams.kstream.Suppressed instead.
Use Admin.create instead of AdminClient.create. Static methods
in Java interfaces can be invoked since Scala 2.12. I noticed that
MirrorMaker 2 uses AdminClient.create, but I did not change them
as Connectors have restrictions on newer client APIs.
Improve efficiency in a few Gauge implementations by avoiding
unnecessary intermediate collections.
Remove pointless Option.apply in ZookeeperClient
SessionState metric.
Fix unused import/variable and other compiler warnings.
Reduce visibility of some vals/defs.

Committer Checklist (excluded from commit message)

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

gwenshap · 2019-12-23T12:19:39Z

Wow. Thanks for the amazing refactoring. Out of curiosity:

The change of SAM types to lambdas was done in Java classes. How is it related to the drop of 2.11 support? Did 2.11 prevent us from using lambda syntax somehow?
How did you find all the SAM types to replace? Is there a linting tool?

gwenshap · 2019-12-23T15:33:31Z

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

-             }
-           })
+  newGauge("cleaner-recopy-percent", () => {
+    val stats = cleaners.map(_.lastStats)


This will still create an intermediate collection, no? Why not use cleaners.iterator.map, turn stats into an iterator... and avoid creating the extra collection completely?

Yes, good catch. I missed that in the initial change. Will update.

I went to make this change and then I remembered why I didn't do it in the first place. The iterator can only be used once and we need two iterations. We would need to allocate two separator iterators. But cleaners should be a small collection (size 1 by default) so the additional code complexity didn't seem worth it.

ijuma · 2019-12-23T16:01:50Z

Thanks for taking a look.

I used an IntelliJ inspection to find candidates. It didn't find most of the Gauge usages, so I relied on plain old search to find those. That's how I also came across the Java examples you asked about. You are correct that the Java changes could have been done independently of dropping support for Scala 2.11.

ijuma · 2019-12-23T20:50:10Z

I noticed that
MirrorMaker 2 uses AdminClient.create, but I did not change them
as Connectors have restrictions on newer client APIs.

@rhauch @ryannedolan Can you please confirm that we should stick with the old AdminClient.create API for MirrorMaker 2?

ijuma · 2019-12-23T21:19:45Z

core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala

-    val alterResourceName = if (resourceName.nonEmpty)
-      resourceName
-    else
-      ConfigEntityName.Default


This was unused.

guozhangwang · 2019-12-23T23:12:32Z

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

@@ -102,7 +102,7 @@ object AclCommand extends Logging {
      else
        new Properties()
      props.put(AdminClientConfig.BOOTSTRAP_SERVERS_CONFIG, opts.options.valueOf(opts.bootstrapServerOpt))
-      val adminClient = JAdminClient.create(props)
+      val adminClient = Admin.create(props)


guozhangwang · 2019-12-23T23:23:07Z

core/src/test/scala/unit/kafka/log/LogManagerTest.scala

-        } else {
-          invocation.callRealMethod().asInstanceOf[Try[File]]
-        }
+    doAnswer { invocation =>


nit: should we use () here? Do we have any non-written rules on lambdas for when () or {} should be used.

Do you mean doAnswer(invocation => {...? If so, I would say no. The block syntax is more concise and generally recommended for cases like this.

guozhangwang · 2019-12-23T23:29:49Z

...ams/streams-scala/src/main/scala/org/apache/kafka/streams/scala/kstream/KGroupedStream.scala

@@ -78,8 +78,6 @@ class KGroupedStream[K, V](val inner: KGroupedStreamJ[K, V]) {
   * @see `org.apache.kafka.streams.kstream.KGroupedStream#reduce`
   */
  def reduce(reducer: (V, V) => V)(implicit materialized: Materialized[K, V, ByteArrayKeyValueStore]): KTable[K, V] =
-    // need this explicit asReducer for Scala 2.11 or else the SAM conversion doesn't take place
-    // works perfectly with Scala 2.12 though


Could we remove the asReducer now? Ditto below.

No, we cannot. The comment is wrong as far as I can tell.

guozhangwang · 2019-12-23T23:30:37Z

streams/streams-scala/src/main/scala/org/apache/kafka/streams/scala/kstream/Suppressed.scala

 */
-// TODO: Deprecate this class if support for Scala 2.11 + Java 1.8 is dropped.
+@deprecated(message = "Use org.apache.kafka.streams.kstream.Suppressed", since = "2.5")


Could we remove this class then?

No, we cannot. The comment is wrong as far as I can tell.

ijuma · 2019-12-24T08:22:39Z

retest this please.

ijuma · 2019-12-25T00:04:29Z

JDK 11 and Scala 2.12 passed, JDK 11 and Scala 2.13 had one flaky failure:

org.apache.kafka.clients.consumer.internals.ConsumerCoordinatorTest.testCloseTimeoutCoordinatorUnavailableForCommit[rebalance protocol = EAGER]

ijuma · 2019-12-25T00:04:42Z

retest this please

ijuma · 2019-12-25T12:35:34Z

core/src/main/scala/kafka/zookeeper/ZooKeeperClient.scala

-  newGauge("SessionState", new Gauge[String] {
-    override def value: String = Option(connectionState.toString).getOrElse("DISCONNECTED")
-  })
+  newGauge("SessionState", () => connectionState.toString)


The Option wrapping doesn't seem to achieve anything here. If connectionState is null, we'd get a NPE. toString should always return a non null value.

Static methods in Java interfaces can be invoked since Scala 2.12.

ijuma · 2019-12-26T07:31:49Z

Tests passed for both jobs that are still enabled: JDK 8 and Scala 2.12 and JDK 11 and Scala 2.13.

@guozhangwang (for Streams) and @omkreddy (for Core) please review when you have a chance.

rhauch · 2019-12-27T20:22:21Z

@ijuma wrote:

I noticed that MirrorMaker 2 uses AdminClient.create, but I did not change them
as Connectors have restrictions on newer client APIs.

@rhauch @ryannedolan Can you please confirm that we should stick with the old AdminClient.create API for MirrorMaker 2?

Is MirrorMaker 2 designed to be installed into older Kafka installations? If so, then we should probably stick with the old AdminClient API for MirrorMaker 2. If not and MirrorMaker 2 should always be used with whatever Kafka installation that includes it, then it can use the newer API.

omkreddy

@ijuma Thanks for the PR. Core related changes looks good to me.

guozhangwang · 2020-01-06T17:22:05Z

LGTM, thanks @ijuma !

ijuma · 2020-01-06T18:52:58Z

Thanks for the reviews, merged to trunk.

This reverts commit 6dc6f6a.

- Revert "KAFKA-9324: Drop support for Scala 2.11 (KIP-531) (apache#7859)" - Fix some issue with scala 2.11 build (eg. scala compile option "-target:jvm-1.8") - Change all java lambda expression to anonymous class This reverts commit 6dc6f6a.

ijuma requested review from omkreddy and guozhangwang December 22, 2019 13:44

gwenshap reviewed Dec 23, 2019

View reviewed changes

ijuma commented Dec 23, 2019

View reviewed changes

guozhangwang reviewed Dec 23, 2019

View reviewed changes

ijuma commented Dec 25, 2019

View reviewed changes

ijuma added 12 commits December 25, 2019 22:00

KAFKA-9324: Drop support for Scala 2.11 (KIP-531)

ce599cc

Simplify Gauge usages with SAMs

b51adb1

Fix spotless errors

6fd0131

Fix compilation error with Scala 2.13

585d6ac

Use Admin.create instead of AdminClient.create from Scala code

627306b

Static methods in Java interfaces can be invoked since Scala 2.12.

Minor formatting fix

6a0c994

Remove unnecessary Option wrapping

6c82c12

Minor formatting fixes

de9dddf

Simplify

a8cdcbb

SAM conversions in ZooKeeperClient

46e00bf

ZooKeeperClient changes to make it compile with Scala 2.13

8f04e36

Fix a few Scala 2.13 compiler warnings

61ab584

ijuma force-pushed the kafka-9324-drop-scala-2.11 branch from 5f1e01a to 61ab584 Compare December 25, 2019 22:00

omkreddy approved these changes Jan 2, 2020

View reviewed changes

ijuma merged commit 6dc6f6a into apache:trunk Jan 6, 2020

ijuma deleted the kafka-9324-drop-scala-2.11 branch January 6, 2020 18:51

omkreddy added a commit to confluentinc/kafka that referenced this pull request Jan 8, 2020

Revert "KAFKA-9324: Drop support for Scala 2.11 (KIP-531) (apache#7859)"

98d6061

This reverts commit 6dc6f6a.

qinghui-xu mentioned this pull request Nov 17, 2020

Add scala 2.11 support for kafka 2.5 criteo-forks/kafka#1

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-9324: Drop support for Scala 2.11 (KIP-531) #7859

KAFKA-9324: Drop support for Scala 2.11 (KIP-531) #7859

ijuma commented Dec 22, 2019 •

edited

Loading

gwenshap commented Dec 23, 2019

gwenshap Dec 23, 2019

ijuma Dec 23, 2019 •

edited

Loading

ijuma Dec 23, 2019

ijuma commented Dec 23, 2019

ijuma commented Dec 23, 2019

ijuma Dec 23, 2019

guozhangwang Dec 23, 2019

guozhangwang Dec 23, 2019

ijuma Dec 25, 2019

guozhangwang Dec 23, 2019

ijuma Dec 24, 2019 •

edited

Loading

guozhangwang Dec 23, 2019

ijuma Dec 24, 2019 •

edited

Loading

ijuma commented Dec 24, 2019

ijuma commented Dec 25, 2019

ijuma commented Dec 25, 2019

ijuma Dec 25, 2019

ijuma commented Dec 26, 2019

rhauch commented Dec 27, 2019

omkreddy left a comment

guozhangwang commented Jan 6, 2020

ijuma commented Jan 6, 2020

KAFKA-9324: Drop support for Scala 2.11 (KIP-531) #7859

KAFKA-9324: Drop support for Scala 2.11 (KIP-531) #7859

Conversation

ijuma commented Dec 22, 2019 • edited Loading

Committer Checklist (excluded from commit message)

gwenshap commented Dec 23, 2019

Choose a reason for hiding this comment

ijuma Dec 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijuma commented Dec 23, 2019

ijuma commented Dec 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijuma Dec 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijuma Dec 24, 2019 • edited Loading

Choose a reason for hiding this comment

ijuma commented Dec 24, 2019

ijuma commented Dec 25, 2019

ijuma commented Dec 25, 2019

Choose a reason for hiding this comment

ijuma commented Dec 26, 2019

rhauch commented Dec 27, 2019

omkreddy left a comment

Choose a reason for hiding this comment

guozhangwang commented Jan 6, 2020

ijuma commented Jan 6, 2020

ijuma commented Dec 22, 2019 •

edited

Loading

ijuma Dec 23, 2019 •

edited

Loading

ijuma Dec 24, 2019 •

edited

Loading

ijuma Dec 24, 2019 •

edited

Loading