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 runtime Java version to Temurin 21.0.1 #19553

Merged
merged 6 commits into from Oct 31, 2023
Merged

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Oct 28, 2023

No description provided.

.java-version Outdated Show resolved Hide resolved
core/docker/Dockerfile Outdated Show resolved Hide resolved
core/trino-server-rpm/src/main/rpm/preinstall Outdated Show resolved Hide resolved
core/trino-server-rpm/src/main/rpm/preinstall Show resolved Hide resolved
@electrum
Copy link
Member

Typo "not longer supported" in commit message should be "no longer supported"

@wendigo wendigo force-pushed the serafin/jdk21-runtime branch 3 times, most recently from ae42400 to 81af60c Compare October 31, 2023 11:44
@wendigo wendigo force-pushed the serafin/jdk21-runtime branch 2 times, most recently from bd8d107 to 21a1149 Compare October 31, 2023 15:33
@wendigo wendigo requested a review from electrum October 31, 2023 17:20
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.
@wendigo wendigo merged commit 82cfb00 into master Oct 31, 2023
5 of 14 checks passed
@wendigo wendigo deleted the serafin/jdk21-runtime branch October 31, 2023 19:28
@github-actions github-actions bot added this to the 432 milestone Oct 31, 2023
@@ -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");
Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

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)

Copy link
Member

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.

Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They were removed

@metalshanked
Copy link

metalshanked commented Nov 10, 2023

Hi @raunaqmorarka @wendigo , Seems like the Docker image OS has switched to RHEL from Debian since upgrading to v432.
This broke few things for our custom image which uses debian utilities and packages.

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!

@wendigo
Copy link
Contributor Author

wendigo commented Nov 10, 2023

@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.

@metalshanked
Copy link

Thanks @wendigo - I will work on updating the images to RHEL.
So would v432 onwards only use RHEL for the forseeable future?

@wendigo
Copy link
Contributor Author

wendigo commented Nov 10, 2023

@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.

@metalshanked
Copy link

@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.

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

5 participants