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.

Sign up for GitHub

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

Jump to bottom

Require JDK 17.0.3 #13014

Merged
merged 3 commits into from Jul 14, 2022
Merged

Require JDK 17.0.3 #13014

merged 3 commits into from Jul 14, 2022

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Jun 28, 2022 •
edited by martint

Documentation

(x) Sufficient documentation is included in this PR.

Release notes

(x) Release notes entries required with the following suggested text:

# General
* Update minimum required Java version to 17.0.3. ({issue}`13014`)

@cla-bot cla-bot bot added the cla-signed label Jun 28, 2022
@wendigo wendigo added tests:hive tests:all Run all tests tests:all-product Run all product tests labels Jun 28, 2022
@tangjiangling
Copy link
Member

@wendigo Do we have any follow-up plans on the TPC-H or TPC-DS tests to see how upgrading JDK17 affects performance?

@wendigo
Copy link
Contributor Author

wendigo commented Jun 28, 2022 •
edited

@tangjiangling we've already benchmarked Trino on JDK 17 and there are minor improvements and no regressions (which is something that we expected).

We are moving to JDK17 to leverage new APIs and language features, rather than expecting some large performance boost.

@martint
Copy link
Member

martint commented Jun 28, 2022

I ran some microbenchmarks a few months ago and observed a 10-15% performance improvement in some code paths, so we expect things to be generally faster.

@wendigo
Copy link
Contributor Author

wendigo commented Jun 28, 2022

trino-hive test failed with OOM. Phoenix5 failed as expected - waiting for 5.2 release, which should fix the underlying issue.

@wendigo wendigo force-pushed the serafin/jdk17-tests branch 2 times, most recently from 91ec1ab to b810916 Compare June 30, 2022 12:07
kokosing
kokosing reviewed Jun 30, 2022
View reviewed changes
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

Possibly commits need to be reordered.

.../src/main/java/org/apache/phoenix/shaded/org/apache/zookeeper/client/StaticHostProvider.java Outdated Show resolved Hide resolved
core/docker/default/etc/jvm.config Outdated Show resolved Hide resolved
lib/trino-phoenix5-patched/pom.xml Outdated Show resolved Hide resolved
.../src/main/java/org/apache/phoenix/shaded/org/apache/zookeeper/client/StaticHostProvider.java Outdated Show resolved Hide resolved
@wendigo wendigo changed the title [WIP] Run all tests on JDK 17 Switch all tests on JDK 17 Jun 30, 2022
@wendigo wendigo changed the title Switch all tests on JDK 17 Switch all tests to JDK 17 Jun 30, 2022
@wendigo wendigo force-pushed the serafin/jdk17-tests branch 3 times, most recently from b992145 to 442106a Compare July 4, 2022 16:29
@wendigo wendigo changed the title Switch all tests to JDK 17 Run all tests to JDK 17 Jul 4, 2022
@mosabua
Copy link
Member

mosabua commented Jul 4, 2022

If we switch to run tests on 17 only (as done in this PR).. we should make 17 a requirement in the docs and the launcher .. otherwise we should run on 11 and 17 ..

Personally I think we should make 17 the requirement in the docs now. And in the launcher use a warning that 11 is deprecated and no longer tested. And then in another few releases .. we make 17 the requirement in the launcher as well.

We have been announcing and messaging the move for a while already.

@wendigo
Copy link
Contributor Author

wendigo commented Jul 4, 2022

Calling @electrum @martint for the decision here

electrum
electrum reviewed Jul 8, 2022
View reviewed changes
core/docker/Dockerfile Outdated Show resolved Hide resolved
@wendigo wendigo force-pushed the serafin/jdk17-tests branch 3 times, most recently from 14771c3 to 79ed19e Compare July 9, 2022 12:52
@wendigo
Copy link
Contributor Author

wendigo commented Jul 9, 2022

JDK 17.0.3 is required. I've switched Docker to build from official azul jdk image.

@wendigo wendigo requested a review from electrum July 9, 2022 12:55
martint
martint reviewed Jul 9, 2022
View reviewed changes
core/docker/Dockerfile Show resolved Hide resolved
core/docker/Dockerfile Outdated Show resolved Hide resolved
core/docker/Dockerfile Show resolved Hide resolved
@wendigo
Copy link
Contributor Author

wendigo commented Jul 12, 2022

@martint I've extracted docker change to a separate commit

@martint
Copy link
Member

martint commented Jul 12, 2022

The first commit should update the docker image to download and install the new JDK. Otherwise, if we do have to back out the second commit for any reason, the docker images will no longer work without additional changes.

@wendigo
Copy link
Contributor Author

wendigo commented Jul 13, 2022

@martint done

@martint martint merged commit da367a3 into trinodb:master Jul 14, 2022
@github-actions github-actions bot added this to the 390 milestone Jul 14, 2022
@wendigo wendigo deleted the serafin/jdk17-tests branch July 14, 2022 17:25
@colebow colebow mentioned this pull request Jul 14, 2022
Merged
@kokosing
Copy link
Member

Yuhuuu! 🎉

findepi
findepi reviewed Jul 19, 2022
View reviewed changes
core/docker/default/etc/jvm.config Show resolved Hide resolved
core/docker/default/etc/jvm.config
-XX:-UseBiasedLocking
-XX:+UseG1GC
-XX:InitialRAMPercentage=80
-XX:MaxRAMPercentage=80
Copy link
Member

Choose a reason for hiding this comment

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

The removal of -Xmx1G doesn't seem related to Java 17 support and may be a breaking change for users.

pom.xml Show resolved Hide resolved
...uct-tests-launcher/src/main/resources/docker/presto-product-tests/conf/presto/etc/jvm.config
@@ -1,6 +1,5 @@
-server
-Xmx2G
-XX:-UseBiasedLocking
-XX:+UseG1GC
Copy link
Member

Choose a reason for hiding this comment

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

remove -XX:+UseG1GC as elsewhere?

@wendigo
Copy link
Contributor Author

wendigo commented Jul 19, 2022

Thx @findepi. I will do a follow up PR with gc flags cleanup

@nineinchnick nineinchnick mentioned this pull request Jul 22, 2022
Merged
@wendigo wendigo mentioned this pull request Jul 23, 2022
Merged
@cho-m cho-m mentioned this pull request Sep 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martint martint martint left review comments

@findepi findepi findepi left review comments

@mosabua mosabua mosabua approved these changes

@ksobolew ksobolew ksobolew left review comments

@kokosing kokosing Awaiting requested review from kokosing

@jirassimok jirassimok Awaiting requested review from jirassimok

@electrum electrum Awaiting requested review from electrum

Assignees
No one assigned
Labels
cla-signed docs tests:all Run all tests tests:all-product Run all product tests
Milestone
390
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@wendigo @tangjiangling @martint @mosabua @electrum @kokosing @findepi @ksobolew @jirassimok