[KYUUBI #5314] Support JDK 21 #6306

dupen01 · 2024-04-15T07:51:55Z

🔍 Description

Issue References 🔗

This pull request fixes #5314

Describe Your Solution 🔧

To support Java 21, it would be necessary to upgrade the spotless-maven-plugin to a more recent version, such as from 2.30.0 to 2.43.0, as per diffplug/spotless#1819.
Correspondingly, googlejavaformat would also need to be upgraded, which would result in the loss of support for Java 8 (as documented in https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md). This upgrade would cause some existing code formatting checks to fail.
Drawing inspiration from the Apache Flink community's approach, I propose skipping the spotless-maven-plugin's check behavior in java-21 profile. Once the Kyuubi community decides to drop support for Java 8, upgrading both spotless-maven-plugin and googlejavaformat to their newer versions could then be considered.

Types of changes 🔖

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Executed mvn clean package -DskipTests locally under a JDK 21 environment, with no exceptions encountered.

Packaged into a tar file and did some basic Spark engine test on my Mac, no exceptions were observed.

Related Unit Tests

Checklist 📝

This patch was not authored or co-authored using Generative Tooling

Be nice. Be informative.

pan3793 · 2024-04-15T07:55:12Z

pom.xml

+                    -Dio.netty.tryReflectionSetAccessible=true</maven.plugin.surefire.argLine>
+                <!-- Current google format does not run on Java 21.
+                     Don't upgrade it in this profile because it formats code differently.
+                     Re-evaluate once support for Java 8 is dropped. -->


can we recover it by overriding <spotless.java.googlejavaformat.version> in this profile?

But if using a new version of googleJavaFormat for code formatting check under Java 21, one problem is that this would cause some existing code formatting checks to fail, like this:

[ERROR] Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.43.0:check (default) on project kyuubi-hive-jdbc: The following files had format violations: [ERROR] src/main/java/org/apache/kyuubi/jdbc/KyuubiDriver.java [ERROR] @@ -17,6 +17,8 @@ [ERROR] [ERROR] package·org.apache.kyuubi.jdbc; [ERROR] [ERROR] -/**·@deprecated·Use·`KyuubiHiveDriver`·instead.·*/ [ERROR] +/** [ERROR] +·*·@deprecated·Use·`KyuubiHiveDriver`·instead. [ERROR] +·*/ [ERROR] @Deprecated [ERROR] public·class·KyuubiDriver·extends·KyuubiHiveDriver·{}

If set check to apply, in which case it would work.

If we accepts those changes, will the Java8 with old googleJavaFormat be happy?

I will take some tests using both Java 8 and Java 21, and then report back here.

I upgraded the versions of spotless and googlejavaformat in the java21 profile and manually executed mvn spotless:apply, some code formats were adjusted by spotless.

But when I switched to Java8 with old googlejavaformat for checking, errors occurred. It seems that the rules of the new version of googlejavaformat are not compatible with the old version. Perhaps before we drop support for Java8, we can only skip spotless's check in Java21. Once we abandon support for Java8, we can upgrade the versions of spotless and googlejavaformat.

thanks for investigating, ~~let's leave a brief comment to explain the decision, i.e. "Skip Java formatting because Java 21 requires a new version of googlejavaformat, but the format rules conflict with the existing one"~~

Oh, you already did that

pan3793 · 2024-04-15T08:23:30Z

pom.xml

+                <maven.compiler.source></maven.compiler.source>
+                <maven.compiler.target></maven.compiler.target>
+                <maven.compiler.release>${java.version}</maven.compiler.release>
+                <maven.plugin.surefire.argLine>-XX:+IgnoreUnrecognizedVMOptions


actually, we can set this property unconditionally, instead of duplicating it in different profiles

pan3793 · 2024-04-15T08:25:41Z

I merged your arrow upgrading PR, please rebase this PR on master, and add a CI coverage for Java 21

dupen01 · 2024-04-15T09:26:18Z

I merged your arrow upgrading PR, please rebase this PR on master, and add a CI coverage for Java 21

Thanks, I will synchronize from the master branch.

pan3793 · 2024-04-15T09:28:07Z

@dupen01 you don't need to recreate new PR, just sync the master branch and perform git rebase master

dupen01 · 2024-04-15T09:28:15Z

Sorry, accidentally clicked Close; please kindly reopen.

pan3793 · 2024-04-16T02:32:23Z

you can add the following part to .github/workflows/master.yml line 74 to add the Java 21 CI

          - java: 21
            spark: '3.5'
            spark-archive: ''
            exclude-tags: ''
            comment: 'normal'

codecov-commenter · 2024-04-16T03:23:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.41%. Comparing base (9086cfe) to head (7544bbe).
Report is 4 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #6306      +/-   ##
============================================
- Coverage     58.47%   58.41%   -0.07%     
  Complexity       24       24              
============================================
  Files           652      652              
  Lines         39645    39645              
  Branches       5454     5454              
============================================
- Hits          23181    23157      -24     
- Misses        13984    13995      +11     
- Partials       2480     2493      +13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dupen01 · 2024-04-16T03:31:03Z

My new submission includes other PR commit, and I'm not sure if there are any conflicts. Could you please review it for me? I apologize for any inconvenience this may cause you.

pan3793 · 2024-04-16T03:40:07Z

My new submission includes other PR commit, and I'm not sure if there are any conflicts

Git is a little bit confusing for new contributors, I hope the following article and tool helps you to learn it.

[1] https://dev.to/lydiahallie/cs-visualized-useful-git-commands-37p1
[2] https://git-fork.com/

pan3793 · 2024-04-16T07:53:29Z

java.lang.IllegalAccessException: class org.apache.kyuubi.client.ServerTestHelper cannot access class sun.security.tools.keytool.CertAndKeyGen (in module java.base) because module java.base does not export sun.security.tools.keytool to unnamed module @b121a8

looks additional module export is required

dupen01 · 2024-04-16T09:32:31Z

java.lang.IllegalAccessException: class org.apache.kyuubi.client.ServerTestHelper cannot access class sun.security.tools.keytool.CertAndKeyGen (in module java.base) because module java.base does not export sun.security.tools.keytool to unnamed module @b121a8

looks additional module export is required

I reproduced the exception, and it appears to be caused by the configuration in the java-17 profile not being passed to java-21. After copying the configuration from the java-17 profile to java-21, the exception disappeared.

<profile>
            <id>java-21</id>
            <activation>
                <jdk>21</jdk>
            </activation>
            <properties>
                <java.version>21</java.version>
                <maven.plugin.surefire.argLine>-XX:+IgnoreUnrecognizedVMOptions
                    --add-opens=java.base/java.lang=ALL-UNNAMED
                    --add-opens=java.base/java.lang.invoke=ALL-UNNAMED
                    --add-opens=java.base/java.lang.reflect=ALL-UNNAMED
                    --add-opens=java.base/java.io=ALL-UNNAMED
                    --add-opens=java.base/java.net=ALL-UNNAMED
                    --add-opens=java.base/java.nio=ALL-UNNAMED
                    --add-opens=java.base/java.util=ALL-UNNAMED
                    --add-opens=java.base/java.util.concurrent=ALL-UNNAMED
                    --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED
                    --add-opens=java.base/sun.nio.ch=ALL-UNNAMED
                    --add-opens=java.base/sun.nio.cs=ALL-UNNAMED
                    --add-opens=java.base/sun.security.action=ALL-UNNAMED
                    --add-opens=java.base/sun.security.tools.keytool=ALL-UNNAMED
                    --add-opens=java.base/sun.security.x509=ALL-UNNAMED
                    --add-opens=java.base/sun.util.calendar=ALL-UNNAMED
                    -Djdk.reflect.useDirectMethodHandle=false
                    -Dio.netty.tryReflectionSetAccessible=true</maven.plugin.surefire.argLine>
            </properties>
            <build>
...

dupen01 · 2024-04-18T08:15:02Z

Hi, @pan3793.
Regarding the two errors that occurred in the CI workflow, here is my investigation:

Scala Test (2.13, 8, 3.5):
Exception message:

Error: ] /home/runner/work/kyuubi/kyuubi/kyuubi-server/src/main/scala/org/apache/kyuubi/server/metadata/jdbc/JDBCMetadataStore.scala:83: Implicit definition should have explicit type (inferred com.zaxxer.hikari.HikariDataSource)

Analysis:
This is likely a new feature of Scala 2.13 that does not support the omission of explicit types when defining implicit variables. When I made the following change to the problematic code kyuubi-server/src/main/scala/org/apache/kyuubi/server/metadata/jdbc/JDBCMetadataStore.scala:

83  -  implicit private[kyuubi] val hikariDataSource = new HikariDataSource(hikariConfig)
83  +  implicit private[kyuubi] val hikariDataSource: HikariDataSource = new HikariDataSource(hikariConfig)

After re-executing the Scala Test (2.13, 8, 3.5) workflow, it succeeded.
This exception seems unrelated to this issue.

Kyuubi and Spark Test (21, 3.5, normal):
Exception message:

- max result rows *** FAILED ***
  org.apache.kyuubi.jdbc.hive.KyuubiSQLException: org.apache.spark.SparkException: Job aborted due to stage failure: Task 0 in stage 68.0 failed 1 times, most recent failure: Lost task 0.0 in stage 68.0 (TID 68) (localhost executor driver): java.lang.UnsupportedOperationException: sun.misc.Unsafe or java.nio.DirectByteBuffer.<init>(long, int) not available
	at org.apache.arrow.memory.util.MemoryUtil.directBuffer(MemoryUtil.java:174)
	at org.apache.arrow.memory.ArrowBuf.getDirectBuffer(ArrowBuf.java:229)
	at org.apache.arrow.memory.ArrowBuf.nioBuffer(ArrowBuf.java:224)
	...

Analysis:
I reproduced this exception locally, and it seems to be caused by a version mismatch between the arrow dependency included in the Spark 3.5 distribution package (version 12.0.0) and in Kyuubi's master branch(15.0.2) .
Testing:
After replacing the built-in arrow-12.0.0 in the Spark 3.5 distribution package with arrow-15.0.2, and performing the "max result rows" test locally, the exception disappeared.

pan3793 · 2024-04-18T08:37:55Z

@dupen01

for issue 1, we can do Scala 2.13 upgrading independently.

issue 2 was mentioned in #5314

replace arrow jars under Spark 3.5 runtime in CI workflow, just as we did for Hive #5280

add support java 21

7544bbe

github-actions bot added the kind:build label Apr 15, 2024

pan3793 reviewed Apr 15, 2024

View reviewed changes

dupen01 closed this Apr 15, 2024

pan3793 reopened this Apr 15, 2024

github-actions bot added kind:documentation Documentation is a feature! kind:infra license, community building, project builds, asf infra related, etc. module:server module:hive module:common module:jdbc module:integration-tests labels Apr 16, 2024

dupen01 force-pushed the issue-java21 branch from 807ffc9 to 7544bbe Compare April 16, 2024 14:31

github-actions bot removed kind:documentation Documentation is a feature! kind:infra license, community building, project builds, asf infra related, etc. module:server module:hive module:common module:jdbc labels Apr 16, 2024

github-actions bot removed the module:integration-tests label Apr 16, 2024

dupeng added 2 commits April 16, 2024 22:40

Merge branch 'master' into issue-java21

5962b6e

add Java 21 CI and update java-21 profile

d28cd2b

github-actions bot added the kind:infra license, community building, project builds, asf infra related, etc. label Apr 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KYUUBI #5314] Support JDK 21 #6306

[KYUUBI #5314] Support JDK 21 #6306

dupen01 commented Apr 15, 2024

pan3793 Apr 15, 2024 •

edited

dupen01 Apr 15, 2024

pan3793 Apr 15, 2024

dupen01 Apr 15, 2024 •

edited

dupen01 Apr 16, 2024

pan3793 Apr 16, 2024 •

edited

pan3793 Apr 15, 2024

pan3793 commented Apr 15, 2024 •

edited

dupen01 commented Apr 15, 2024

pan3793 commented Apr 15, 2024

dupen01 commented Apr 15, 2024

pan3793 commented Apr 16, 2024

codecov-commenter commented Apr 16, 2024 •

edited

dupen01 commented Apr 16, 2024

pan3793 commented Apr 16, 2024

pan3793 commented Apr 16, 2024

dupen01 commented Apr 16, 2024

dupen01 commented Apr 18, 2024

pan3793 commented Apr 18, 2024

[KYUUBI #5314] Support JDK 21 #6306

Are you sure you want to change the base?

[KYUUBI #5314] Support JDK 21 #6306

Conversation

dupen01 commented Apr 15, 2024

🔍 Description

Issue References 🔗

Describe Your Solution 🔧

Types of changes 🔖

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests

Checklist 📝

pan3793 Apr 15, 2024 • edited

Choose a reason for hiding this comment

dupen01 Apr 15, 2024

Choose a reason for hiding this comment

pan3793 Apr 15, 2024

Choose a reason for hiding this comment

dupen01 Apr 15, 2024 • edited

Choose a reason for hiding this comment

dupen01 Apr 16, 2024

Choose a reason for hiding this comment

pan3793 Apr 16, 2024 • edited

Choose a reason for hiding this comment

pan3793 Apr 15, 2024

Choose a reason for hiding this comment

pan3793 commented Apr 15, 2024 • edited

dupen01 commented Apr 15, 2024

pan3793 commented Apr 15, 2024

dupen01 commented Apr 15, 2024

pan3793 commented Apr 16, 2024

codecov-commenter commented Apr 16, 2024 • edited

Codecov Report

dupen01 commented Apr 16, 2024

pan3793 commented Apr 16, 2024

pan3793 commented Apr 16, 2024

dupen01 commented Apr 16, 2024

dupen01 commented Apr 18, 2024

pan3793 commented Apr 18, 2024

pan3793 Apr 15, 2024 •

edited

dupen01 Apr 15, 2024 •

edited

pan3793 Apr 16, 2024 •

edited

pan3793 commented Apr 15, 2024 •

edited

codecov-commenter commented Apr 16, 2024 •

edited