-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 runtime Java version to Temurin 21.0.1 #19553
Conversation
f01d9cc
to
d9b8d89
Compare
d9b8d89
to
ffd6f92
Compare
core/trino-main/src/main/java/io/trino/server/TrinoSystemRequirements.java
Outdated
Show resolved
Hide resolved
Typo "not longer supported" in commit message should be "no longer supported" |
ae42400
to
81af60c
Compare
bd8d107
to
21a1149
Compare
This commit introduces following changes: * All Trino tests runs on JDK 21 (including product tests) * Recommend Eclipse Temurin in RPM preinstall * Add testing of RPM with JDK 21 * Switches docker image to use JDK 21 This commit does not change the required runtime JDK version and doesn't change the language level to 21. This allows this change to be reverted if regressions are reported.
21a1149
to
315f8f8
Compare
@@ -93,7 +93,7 @@ else if ("Mac OS X".equals(osName)) { | |||
|
|||
private static void verifyJavaVersion() | |||
{ | |||
Version required = Version.parse("17.0.3"); | |||
Version required = Version.parse("17.0.5"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why wasn't this also updated to require Java 21 ?
Since we've removed certain JVM configs (and related documentation) which were helpful on older versions like -XX:+UseAESCTRIntrinsics
and -XX:-G1UsePreventiveGC
, but aren't requiring Java 21, we now have the possibility of deployments which will use older JVM but miss these configs.
cc: @sopel39 @lukasz-stec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would force JDK 21 usage. If you want to use JDK 17 you need to use old configs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would force JDK 21 usage
I know it would force JDK 21 usage, I'm asking why not ?
If you want to use JDK 17 you need to use old configs
How does a user know which old configs should be used given we've scrubbed all mention of them from the repo ?
If we want to support both Java 17 and Java 21 simultaneously for some time, then our docs should clarify this and maintain the previous recommendations for Java 17 jvm configs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raunaqmorarka existing other-than-docker deployments can still work without any changes if you stick to 17 and we can always revert (probability is low but still possible)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that existing setups will continue to work, my concern is about new other-than-docker deployments, they would miss the old recommended JVM configs and would likely deploy with Java 17 as Java 21 is still very new.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we've removed certain JVM configs (and related documentation) which were helpful on older versions like -XX:+UseAESCTRIntrinsics and -XX:-G1UsePreventiveGC
Can we keep them even in 21 or they were completely removed in newest version of Java? I don't think they cause harm in 21
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They were removed
Hi @raunaqmorarka @wendigo , Seems like the Docker image OS has switched to RHEL from Debian since upgrading to v432. Can you please advise if the official image would be switched back to Debian in the near future or are there alternate official images with Debian available ? Thanks! |
@metalshanked if you need a custom image, you can build one using tar.gz archive that we publish (i.e. https://repo1.maven.org/maven2/io/trino/trino-server/432/trino-server-432.tar.gz) and base image of your choosing. We want to provide as small and as secure image as possible that is RHEL-compatible. |
Thanks @wendigo - I will work on updating the images to RHEL. |
@metalshanked I'd say that we don't guarantee any specific distro and tooling in the container. We guarantee only that Trino will be there and we test whether it works with the base OS. |
Thanks. Will setup a probe to check base os for every release. |
No description provided.