Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to JDK 22 in the runtime #21161

Merged
merged 2 commits into from Mar 25, 2024
Merged

Switch to JDK 22 in the runtime #21161

merged 2 commits into from Mar 25, 2024

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Mar 19, 2024

Release notes: use Java 22 in the docker image.

@cla-bot cla-bot bot added the cla-signed label Mar 19, 2024
@wendigo wendigo mentioned this pull request Mar 19, 2024
12 tasks
@wendigo
Copy link
Contributor Author

wendigo commented Mar 19, 2024

Failures so far:

Alluxio (ok, will go away with the 22 release):

2024-03-19T13:39:33.8546992Z Caused by: java.lang.ExceptionInInitializerError: Exception java.lang.NumberFormatException: For input string: "22-beta" [in thread "ForkJoinPool-1-worker-3"]

ServerIT (ok, will update to 22):

  • wrong docker image (eclipse-temurin:21--re-ubi9-minimal)

@wendigo
Copy link
Contributor Author

wendigo commented Mar 19, 2024

cc @electrum @martint @mosabua

@wendigo wendigo force-pushed the serafin/jdk-22-ready branch 5 times, most recently from 8faa202 to 862eae8 Compare March 21, 2024 18:00
@wendigo wendigo force-pushed the serafin/jdk-22-ready branch 3 times, most recently from 6cc1630 to 31bbbf0 Compare March 21, 2024 21:47
@wendigo wendigo changed the title Test: JDK language level 22 with Temurin 22-ea DONOTMERGE: Switch to JDK 22 in the runtime Mar 21, 2024
@wendigo wendigo requested a review from electrum March 21, 2024 22:31
@wendigo wendigo changed the title DONOTMERGE: Switch to JDK 22 in the runtime Switch to JDK 22 in the runtime Mar 22, 2024
@wendigo wendigo requested a review from martint March 22, 2024 06:16
@wendigo wendigo force-pushed the serafin/jdk-22-ready branch 2 times, most recently from 87d7a64 to 311c5bf Compare March 22, 2024 13:27
@wendigo
Copy link
Contributor Author

wendigo commented Mar 22, 2024

@electrum PTAL for raptor changes

@wendigo wendigo requested a review from findepi March 22, 2024 19:28
@findepi
Copy link
Member

findepi commented Mar 22, 2024

No release notes needed.

Do not merge.

are both still accurate?

@wendigo
Copy link
Contributor Author

wendigo commented Mar 22, 2024

@findepi updated

.java-version Show resolved Hide resolved
@wendigo
Copy link
Contributor Author

wendigo commented Mar 22, 2024

Just rebased

@wendigo
Copy link
Contributor Author

wendigo commented Mar 25, 2024

@martint @electrum PTAL

@electrum electrum merged commit 0183193 into master Mar 25, 2024
102 checks passed
@electrum electrum deleted the serafin/jdk-22-ready branch March 25, 2024 19:17
@github-actions github-actions bot added this to the 444 milestone Mar 25, 2024
@@ -76,6 +76,9 @@ public void testInstallUninstall()
// Release names as in the https://api.adoptium.net/q/swagger-ui/#/Release%20Info/getReleaseNames
testInstall("jdk-21.0.2+13", "/usr/lib/jvm/temurin-21", "21");
testUninstall("jdk-21.0.2+13", "/usr/lib/jvm/temurin-21", "21");

testInstall("jdk-22+36", "/usr/lib/jvm/temurin-22", "22");
testUninstall("jdk-22+36", "/usr/lib/jvm/temurin-22", "22");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expectedJavaVersion parameter in testUninstall is not used, and Error Prone fails the build for me locally on this. Why isn't it failing on CI?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because it skips some mdules -

$MAVEN ${MAVEN_TEST} -T 1C clean verify -DskipTests ${MAVEN_GIB} -Dgib.buildUpstream=never -P errorprone-compiler \
-pl '!:trino-docs,!:trino-server,!:trino-server-rpm'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's fix both issues

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why though. It's been there since beginning - 7949e41

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we thought that there's no Java code in that module (like it's the case for docs and rpm modules)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, I now realized that this is the RPM module :D

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But yeah, I too would assume that there's no Java code here

@covalesj
Copy link

covalesj commented May 9, 2024

Just a note -- jdk22 is a short term release; depending on that is going to cause issues in some enterprise environments that only stick to LTS versions of java. Only a problem if jdk22 features begin to get baseline, which will result in forcing longish upgrade cycles for enterprise customers.

@mosabua
Copy link
Member

mosabua commented May 9, 2024

@covalesj we are aware of that. However the benefits outweigh these issues. We will continue to stay with the latest Java release going forward. What JDK is used is mostly an implementation detail. Especially in container-based deployments there is no impact. For non-container based deployments users will just have to choose to stay with old Trino and old Java .. or go with new features, more performance, timely security fixes, bug fixes and overall improvements all around.

Also keep in mind that the "support" window for Java release is completely variable between vendors and you can get support for any release. We are using Eclipse Temurin as an open source project for our testing and inclusion in the container.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

7 participants