You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/06/28 14:12:00 UTC
[GitHub] [kafka] jlprat opened a new pull request #10934: [DO NOT MERGE] Scala3 test
jlprat opened a new pull request #10934:
URL: https://github.com/apache/kafka/pull/10934
This is a draft PR to showcase how complicated is to migrate to Scala3.
It includes a necessary change as well (removal of Scala Collections Compat and Java8 Compat libraries). This is needed as those libraries' only purpose was to cross-compile to Scala 2.11 and 2.12.
After this is done, only a bit of workaround gradle was needed (library names for Scala 3 compiler changed).
The only code change needed is the renaming of methods like `asJava`, `asScala`, `until`, `from`, and `to` which were since Scala 2.13.0 (Compat libraries were shadowing those). And this change is pretty automatic.
On the test file (only 1 didn't really compile) only the right new import needed to be brought in.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jlprat commented on a change in pull request #10934: [DO NOT MERGE] Scala3 test
Posted by GitBox <gi...@apache.org>.
jlprat commented on a change in pull request #10934:
URL: https://github.com/apache/kafka/pull/10934#discussion_r659837190
##########
File path: gradle/dependencies.gradle
##########
@@ -179,10 +183,10 @@ libs += [
powermockEasymock: "org.powermock:powermock-api-easymock:$versions.powermock",
reflections: "org.reflections:reflections:$versions.reflections",
rocksDBJni: "org.rocksdb:rocksdbjni:$versions.rocksDB",
- scalaCollectionCompat: "org.scala-lang.modules:scala-collection-compat_$versions.baseScala:$versions.scalaCollectionCompat",
- scalaJava8Compat: "org.scala-lang.modules:scala-java8-compat_$versions.baseScala:$versions.scalaJava8Compat",
scalaLibrary: "org.scala-lang:scala-library:$versions.scala",
- scalaLogging: "com.typesafe.scala-logging:scala-logging_$versions.baseScala:$versions.scalaLogging",
+ scala3Library: "org.scala-lang:scala3-library_3:3.0.0",
Review comment:
I had something like this on my early attempts, but I went for this as it was "clearer" for the reviewer. I go back to that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jlprat commented on a change in pull request #10934: [DO NOT MERGE] Scala3 test
Posted by GitBox <gi...@apache.org>.
jlprat commented on a change in pull request #10934:
URL: https://github.com/apache/kafka/pull/10934#discussion_r659868458
##########
File path: gradle/dependencies.gradle
##########
@@ -23,22 +23,26 @@ ext {
// Available if -PscalaVersion is used. This is useful when we want to support a Scala version that has
// a higher minimum Java requirement than Kafka. This was previously the case for Scala 2.12 and Java 7.
- availableScalaVersions = [ '2.12', '2.13' ]
+ availableScalaVersions = [ '2.13', '3.0' ]
}
// Add Scala version
-def defaultScala212Version = '2.12.14'
def defaultScala213Version = '2.13.6'
+def defaultScala30Version = '3.0.0'
if (hasProperty('scalaVersion')) {
- if (scalaVersion == '2.12') {
- versions["scala"] = defaultScala212Version
- } else if (scalaVersion == '2.13') {
+ if (scalaVersion == '2.13') {
versions["scala"] = defaultScala213Version
+ } else if (scalaVersion == '3.0') {
+ versions["scala"] = defaultScala30Version
+ versions["baseScala"] = '3'
+ versions["baseScalaJackson"] = '2.13'
+ versions["postfixScala"] = '3'
+ versions["postfixScalaOptionalVersion"] = '_3'
Review comment:
@ijuma that is what I meant with not so readable. As both prefixes are only present on Scala 3 and they have different syntax (one with `_`, the other without) I need this somewhat ugly code.
Please note, this might go away eventually if Gradle supports Scala 3: https://github.com/gradle/gradle/issues/16527
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jlprat commented on a change in pull request #10934: [DO NOT MERGE] Scala3 test
Posted by GitBox <gi...@apache.org>.
jlprat commented on a change in pull request #10934:
URL: https://github.com/apache/kafka/pull/10934#discussion_r659831262
##########
File path: gradle/dependencies.gradle
##########
@@ -179,10 +183,10 @@ libs += [
powermockEasymock: "org.powermock:powermock-api-easymock:$versions.powermock",
reflections: "org.reflections:reflections:$versions.reflections",
rocksDBJni: "org.rocksdb:rocksdbjni:$versions.rocksDB",
- scalaCollectionCompat: "org.scala-lang.modules:scala-collection-compat_$versions.baseScala:$versions.scalaCollectionCompat",
- scalaJava8Compat: "org.scala-lang.modules:scala-java8-compat_$versions.baseScala:$versions.scalaJava8Compat",
scalaLibrary: "org.scala-lang:scala-library:$versions.scala",
- scalaLogging: "com.typesafe.scala-logging:scala-logging_$versions.baseScala:$versions.scalaLogging",
+ scala3Library: "org.scala-lang:scala3-library_3:3.0.0",
Review comment:
I can add a couple of extra variables, but please notice that the library changed in the name as well:
it's `scala3-library_3:3.0.0` vs. `scala-library_2.13:2:13.16
Notice the extra `3` right after `scala`.
##########
File path: gradle/dependencies.gradle
##########
@@ -179,10 +183,10 @@ libs += [
powermockEasymock: "org.powermock:powermock-api-easymock:$versions.powermock",
reflections: "org.reflections:reflections:$versions.reflections",
rocksDBJni: "org.rocksdb:rocksdbjni:$versions.rocksDB",
- scalaCollectionCompat: "org.scala-lang.modules:scala-collection-compat_$versions.baseScala:$versions.scalaCollectionCompat",
- scalaJava8Compat: "org.scala-lang.modules:scala-java8-compat_$versions.baseScala:$versions.scalaJava8Compat",
scalaLibrary: "org.scala-lang:scala-library:$versions.scala",
- scalaLogging: "com.typesafe.scala-logging:scala-logging_$versions.baseScala:$versions.scalaLogging",
+ scala3Library: "org.scala-lang:scala3-library_3:3.0.0",
Review comment:
I had something like this on my early attempts, but I went for this as it was "clearer" for the reviewer. I go back to that
##########
File path: gradle/dependencies.gradle
##########
@@ -23,22 +23,26 @@ ext {
// Available if -PscalaVersion is used. This is useful when we want to support a Scala version that has
// a higher minimum Java requirement than Kafka. This was previously the case for Scala 2.12 and Java 7.
- availableScalaVersions = [ '2.12', '2.13' ]
+ availableScalaVersions = [ '2.13', '3.0' ]
}
// Add Scala version
-def defaultScala212Version = '2.12.14'
def defaultScala213Version = '2.13.6'
+def defaultScala30Version = '3.0.0'
if (hasProperty('scalaVersion')) {
- if (scalaVersion == '2.12') {
- versions["scala"] = defaultScala212Version
- } else if (scalaVersion == '2.13') {
+ if (scalaVersion == '2.13') {
versions["scala"] = defaultScala213Version
+ } else if (scalaVersion == '3.0') {
+ versions["scala"] = defaultScala30Version
+ versions["baseScala"] = '3'
+ versions["baseScalaJackson"] = '2.13'
+ versions["postfixScala"] = '3'
+ versions["postfixScalaOptionalVersion"] = '_3'
Review comment:
@ijuma that is what I meant with not so readable. As both prefixes are only present on Scala 3 and they have different syntax (one with `_`, the other without) I need this somewhat ugly code.
Please note, this might go away eventually if Gradle supports Scala 3: https://github.com/gradle/gradle/issues/16527
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jlprat closed pull request #10934: [DO NOT MERGE] Scala3 test
Posted by GitBox <gi...@apache.org>.
jlprat closed pull request #10934:
URL: https://github.com/apache/kafka/pull/10934
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10934: [DO NOT MERGE] Scala3 test
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10934:
URL: https://github.com/apache/kafka/pull/10934#discussion_r659832754
##########
File path: gradle/dependencies.gradle
##########
@@ -179,10 +183,10 @@ libs += [
powermockEasymock: "org.powermock:powermock-api-easymock:$versions.powermock",
reflections: "org.reflections:reflections:$versions.reflections",
rocksDBJni: "org.rocksdb:rocksdbjni:$versions.rocksDB",
- scalaCollectionCompat: "org.scala-lang.modules:scala-collection-compat_$versions.baseScala:$versions.scalaCollectionCompat",
- scalaJava8Compat: "org.scala-lang.modules:scala-java8-compat_$versions.baseScala:$versions.scalaJava8Compat",
scalaLibrary: "org.scala-lang:scala-library:$versions.scala",
- scalaLogging: "com.typesafe.scala-logging:scala-logging_$versions.baseScala:$versions.scalaLogging",
+ scala3Library: "org.scala-lang:scala3-library_3:3.0.0",
Review comment:
I know, but I don't see the value of exposing that outside of this file. We only use one scalaLibrary for a given build.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jlprat commented on pull request #10934: [DO NOT MERGE] Scala3 test
Posted by GitBox <gi...@apache.org>.
jlprat commented on pull request #10934:
URL: https://github.com/apache/kafka/pull/10934#issuecomment-879108529
Closing in favor of https://github.com/apache/kafka/pull/11025
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jlprat commented on pull request #10934: [DO NOT MERGE] Scala3 test
Posted by GitBox <gi...@apache.org>.
jlprat commented on pull request #10934:
URL: https://github.com/apache/kafka/pull/10934#issuecomment-869729338
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jlprat commented on pull request #10934: [DO NOT MERGE] Scala3 test
Posted by GitBox <gi...@apache.org>.
jlprat commented on pull request #10934:
URL: https://github.com/apache/kafka/pull/10934#issuecomment-869779558
> Thanks for the PR. If I understand correctly, this assumes we would drop Scala 2.12 compatibility. That makes sense, but it does mean waiting quite a while (12+ months).
Shall I try to work on supporting the 3 Scala versions?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10934: [DO NOT MERGE] Scala3 test
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10934:
URL: https://github.com/apache/kafka/pull/10934#discussion_r659829361
##########
File path: gradle/dependencies.gradle
##########
@@ -179,10 +183,10 @@ libs += [
powermockEasymock: "org.powermock:powermock-api-easymock:$versions.powermock",
reflections: "org.reflections:reflections:$versions.reflections",
rocksDBJni: "org.rocksdb:rocksdbjni:$versions.rocksDB",
- scalaCollectionCompat: "org.scala-lang.modules:scala-collection-compat_$versions.baseScala:$versions.scalaCollectionCompat",
- scalaJava8Compat: "org.scala-lang.modules:scala-java8-compat_$versions.baseScala:$versions.scalaJava8Compat",
scalaLibrary: "org.scala-lang:scala-library:$versions.scala",
- scalaLogging: "com.typesafe.scala-logging:scala-logging_$versions.baseScala:$versions.scalaLogging",
+ scala3Library: "org.scala-lang:scala3-library_3:3.0.0",
Review comment:
Why can we not set scalaLibrary with the correct name depending on the version? That would make a lot of changes in `build.gradle` unnecessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #10934: [DO NOT MERGE] Scala3 test
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10934:
URL: https://github.com/apache/kafka/pull/10934#issuecomment-869727682
Thanks for the PR. If I understand correctly, this assumes we would drop Scala 2.12 compatibility. That makes sense, but it does mean waiting quite a while (12+ months).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jlprat commented on a change in pull request #10934: [DO NOT MERGE] Scala3 test
Posted by GitBox <gi...@apache.org>.
jlprat commented on a change in pull request #10934:
URL: https://github.com/apache/kafka/pull/10934#discussion_r659831262
##########
File path: gradle/dependencies.gradle
##########
@@ -179,10 +183,10 @@ libs += [
powermockEasymock: "org.powermock:powermock-api-easymock:$versions.powermock",
reflections: "org.reflections:reflections:$versions.reflections",
rocksDBJni: "org.rocksdb:rocksdbjni:$versions.rocksDB",
- scalaCollectionCompat: "org.scala-lang.modules:scala-collection-compat_$versions.baseScala:$versions.scalaCollectionCompat",
- scalaJava8Compat: "org.scala-lang.modules:scala-java8-compat_$versions.baseScala:$versions.scalaJava8Compat",
scalaLibrary: "org.scala-lang:scala-library:$versions.scala",
- scalaLogging: "com.typesafe.scala-logging:scala-logging_$versions.baseScala:$versions.scalaLogging",
+ scala3Library: "org.scala-lang:scala3-library_3:3.0.0",
Review comment:
I can add a couple of extra variables, but please notice that the library changed in the name as well:
it's `scala3-library_3:3.0.0` vs. `scala-library_2.13:2:13.16
Notice the extra `3` right after `scala`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #10934: [DO NOT MERGE] Scala3 test
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10934:
URL: https://github.com/apache/kafka/pull/10934#issuecomment-869727682
Thanks for the PR. If I understand correctly, this assumes we would drop Scala 2.12 compatibility. That makes sense, but it does mean waiting quite a while (12+ months).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jlprat commented on pull request #10934: [DO NOT MERGE] Scala3 test
Posted by GitBox <gi...@apache.org>.
jlprat commented on pull request #10934:
URL: https://github.com/apache/kafka/pull/10934#issuecomment-869729338
Yes, you are right, this assumes drop of Scala 2.12 compatibility. I did it this way because it was quicker to offer. We could probably still make it work with Scala 2.12, but probably might need a couple of extra changes for all the versions to co-exist. I can also look into it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10934: [DO NOT MERGE] Scala3 test
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10934:
URL: https://github.com/apache/kafka/pull/10934#discussion_r659829361
##########
File path: gradle/dependencies.gradle
##########
@@ -179,10 +183,10 @@ libs += [
powermockEasymock: "org.powermock:powermock-api-easymock:$versions.powermock",
reflections: "org.reflections:reflections:$versions.reflections",
rocksDBJni: "org.rocksdb:rocksdbjni:$versions.rocksDB",
- scalaCollectionCompat: "org.scala-lang.modules:scala-collection-compat_$versions.baseScala:$versions.scalaCollectionCompat",
- scalaJava8Compat: "org.scala-lang.modules:scala-java8-compat_$versions.baseScala:$versions.scalaJava8Compat",
scalaLibrary: "org.scala-lang:scala-library:$versions.scala",
- scalaLogging: "com.typesafe.scala-logging:scala-logging_$versions.baseScala:$versions.scalaLogging",
+ scala3Library: "org.scala-lang:scala3-library_3:3.0.0",
Review comment:
Why can we not set scalaLibrary with the correct name depending on the version? That would make a lot of changes in `build.gradle` unnecessary.
##########
File path: gradle/dependencies.gradle
##########
@@ -179,10 +183,10 @@ libs += [
powermockEasymock: "org.powermock:powermock-api-easymock:$versions.powermock",
reflections: "org.reflections:reflections:$versions.reflections",
rocksDBJni: "org.rocksdb:rocksdbjni:$versions.rocksDB",
- scalaCollectionCompat: "org.scala-lang.modules:scala-collection-compat_$versions.baseScala:$versions.scalaCollectionCompat",
- scalaJava8Compat: "org.scala-lang.modules:scala-java8-compat_$versions.baseScala:$versions.scalaJava8Compat",
scalaLibrary: "org.scala-lang:scala-library:$versions.scala",
- scalaLogging: "com.typesafe.scala-logging:scala-logging_$versions.baseScala:$versions.scalaLogging",
+ scala3Library: "org.scala-lang:scala3-library_3:3.0.0",
Review comment:
I know, but I don't see the value of exposing that outside of this file. We only use one scalaLibrary for a given build.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org