-
Notifications
You must be signed in to change notification settings - Fork 7k
BLD: Switch to JDK 17 from JDK 8 #56281
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Gagandeep Singh <[email protected]>
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.
Code Review
This pull request updates the JDK version from 8 to 17 across various CI/CD scripts and Dockerfiles. The changes are mostly correct, but I've found a couple of issues. There is a critical issue in ci/env/install-hdfs.sh where JAVA_HOME is not updated to the new JDK path; I've added a specific comment for that. Additionally, java/build-jar-multiplatform.sh contains a function check_java_version which still enforces JDK 8. This will likely cause build failures and should be updated to check for JDK 17. Finally, please ensure that the pom.xml files have been updated to target Java 17 (e.g., by setting <maven.compiler.source>17</maven.compiler.source> and <maven.compiler.target>17</maven.compiler.target>), as these files were not part of the pull request and might need changes.
Signed-off-by: czgdp1807 <[email protected]>
|
see #56620 for help on java test reproduce |
aslonnie
left a comment
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.
seems that tests are failing. do we have an idea on why it is failing?
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
|
This pull request has been automatically closed because there has been no more activity in the 14 days Please feel free to reopen or open a new pull request if you'd still like this to be addressed. Again, you can always ask for help on our discussion forum or Ray's public slack channel. Thanks again for your contribution! |
Why are these changes needed?
Except
ci/build/build-manylinux-forge.sh(seems likeyumhas JDK 11 at max), I am able to switch to JDK 17.cc: @aslonnie
Related issue number
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.