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

THRIFT-5644: java to use jdk 17 and upgrade minimal language level to 11 #2686

Merged
merged 1 commit into from
Oct 14, 2022
Merged

THRIFT-5644: java to use jdk 17 and upgrade minimal language level to 11 #2686

merged 1 commit into from
Oct 14, 2022

Conversation

jimexist
Copy link
Member

@jimexist jimexist commented Sep 28, 2022 •
edited
Loading

java to use jdk 17, this is a second try after #2662

  • Did you create an Apache Jira ticket? (not required for trivial changes)
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
  • If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

@jimexist jimexist changed the title java to use jdk 17 THRIFT-5644: java to use jdk 17 Sep 28, 2022
@jimexist
Copy link
Member Author

even if the cross tests might see some errors given they are not related to this fix i'll continue to merge this as a fix because it has resolved the long standing Java TLS error

ctubbsii
ctubbsii reviewed Sep 29, 2022
View reviewed changes
Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

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

I think it's reasonable to use the latest LTS Java for development on the next version of Thrift. I also think it would be reasonable to make Java 11 the minimum instead of 8 at this point.

lib/java/gradle/sourceConfiguration.gradle Outdated Show resolved Hide resolved
@jimexist
Copy link
Member Author

turns out that upgrading to java 17 won't solve the TSL runtime error during cross test

@jimexist jimexist requested a review from ctubbsii October 7, 2022 09:16
@jimexist jimexist changed the title THRIFT-5644: java to use jdk 17 THRIFT-5644: java to use jdk 17 and upgrade minimal language level to 11 Oct 7, 2022
@jimexist
Copy link
Member Author

jimexist commented Oct 7, 2022

because we want to push for raising minimal Java version to 11 we'd need to base on #2690

@jimexist jimexist force-pushed the java-use-jdk-17 branch 2 times, most recently from 03250b0 to a54a7b5 Compare October 10, 2022 00:56
@jimexist jimexist requested a review from Jens-G October 10, 2022 00:56
@jimexist
Copy link
Member Author

@fishy @ctubbsii i have updated the minimal JDK requirement to be 11, please take a look

@jimexist jimexist mentioned this pull request Oct 12, 2022
Merged
5 tasks
ctubbsii
ctubbsii reviewed Oct 13, 2022
View reviewed changes
Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

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

I'm in favor of bumping the min java to 11 for runtime, and 17 for build time. I'm not sure I understand the issue with the cross test failure, but it seems like that's a separate issue from this PR.

fishy
fishy approved these changes Oct 13, 2022
View reviewed changes
build/docker/README.md
@@ -180,7 +180,7 @@ Last updated: October 1, 2017
| erlang | OTP-18 | OTP-23 | |
| go | 1.15.10 | 1.16.2 | |
| haxe | 3.2.1 | 3.4.4 | THRIFT-4352: avoid 3.4.2 |
| java | 1.8.0\_191 | 11.0.3 | |
| java | 1.8.0\_191 | 17 | |
Copy link
Member

Choose a reason for hiding this comment

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

we should also update the min version here? I think we dropped support to jdk8 with this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

this column is for xenial but we have deprecated it recently...

jimexist
jimexist commented Oct 14, 2022
View reviewed changes
lib/java/Makefile.am Outdated
Comment on lines 26 to 27
--console=plain \
--stacktrace
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
--console=plain \
--stacktrace
--console=plain

lib/java/gradle/sourceConfiguration.gradle Outdated Show resolved Hide resolved
build/docker/README.md
@@ -180,7 +180,7 @@ Last updated: October 1, 2017
| erlang | OTP-18 | OTP-23 | |
| go | 1.15.10 | 1.16.2 | |
| haxe | 3.2.1 | 3.4.4 | THRIFT-4352: avoid 3.4.2 |
| java | 1.8.0\_191 | 11.0.3 | |
| java | 1.8.0\_191 | 17 | |
Copy link
Member Author

Choose a reason for hiding this comment

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

this column is for xenial but we have deprecated it recently...

@jimexist jimexist merged commit 92b007f into apache:master Oct 14, 2022
@jimexist jimexist deleted the java-use-jdk-17 branch October 14, 2022 05:16
@ctubbsii ctubbsii mentioned this pull request Mar 2, 2023
Merged
5 tasks
@jimexist jimexist mentioned this pull request Sep 19, 2023
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ctubbsii ctubbsii ctubbsii left review comments

@fishy fishy fishy approved these changes

@Jens-G Jens-G Awaiting requested review from Jens-G

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@jimexist @fishy @ctubbsii