-
Notifications
You must be signed in to change notification settings - Fork 300
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
[RFC] JDK 17 Upgrade & JDK LTS #2295
Comments
Since we will still need to support JDK8, by my understanding we can't use features provided later than that? So we just need to make all the connectors compilable using JDK 17? |
I think that's right, @aimethed. Until we can actually retire JDK8, we only need to build in JDK17. |
@macohen - could you please assign this ticket to me. |
@abhishekpoddar-trianz, please ask any questions for clarification here. I think the priority is producing a build with JDK17. If we need to JDK features so it can run on a JDK8 VM instead of producing multiple builds, that's OK. |
While building with JDK 17, build was failing with below error message: We have fixed build issue by adding java environment variable to yaml file (JAVA_TOOL_OPTIONS: "--add-opens=java.base/java.nio=ALL-UNNAMED") as per the Apache Arrow recommendation https://arrow.apache.org/docs/java/install.html. and we have added --add-opens=java.base/java.nio=ALL-UNNAMED to maven-surefire-plugin and maven-failsafe-plugin in pom.xml file as per the Apache Arrow recommendation https://arrow.apache.org/docs/java/install.html#java-install-maven-testing. After adding these changes, we are able to build successfully. Now we are working on functional testing. |
Fantastic. Adding @mschoeni1 to work on this with you as we move along. |
Can you put up a draft PR with the changes made so far, please, @VenkatasivareddyTR? |
@macohen We have completed functional testing for 4 connectors i.e., PostgreSQL, DynamoDB, MSK, SQL Server and we didn't observe any issues. Please find attached test results for the same. |
@macohen We have completed coding for jdk17 upgrade and published PR as well, can someone review it; We have done functional testing for 5 connectors i.e., PostgreSQL, DynamoDB, MSK, SQL Server, DocDB and build is successful for all connectors along with unit test cases. |
I think we also need to keep the default on JDK11 for now until we have built the connectors with JDK17 a few times and while some other dependencies still require JDK11. |
To clarify, based on a call we had, I'm asking for a proposal to make the JDK version a build time variable. We should be able to use an environment variable to build the connectors for any supported version either inside GH actions or directly with maven or another script locally. |
The PR is updated to get the JDK version as build argument while building the docker image for snowflake. If this approach is fine then we can do the same for all other connectors. Example of build command below. Build cmd: docker build --build-arg JAVA_VERSION=17 --build-arg JAVA_TOOL_OPTIONS="--add-opens=java.base/java.nio=ALL-UNNAMED" -t athena-federation-repository-snowflake:2022.47.1 |
The latest changes look good, thanks. Please go ahead and do the same for the remaining connectors. |
The PR is updated for all the connectors for dynamic update of JDK 17 (JDK version as build argument). It is building successfully and all the unit TCs are passing in our local environment. We have also done the functional testing of 5 connectors and all looks good. PFA the test results. Request you to review the latest changes in the PR: DDB_FUNCTIONAL_TEST_2025-01-13_13_00_31.573309.csv |
Is your feature request related to a problem? If yes, please describe.
This project builds using JDK 8 and 11 today. Updating to JDK 17 would help modernize the connectors and create a clearer path to stay modern and update to the latest JDKs regularly.
Describe the solution you'd like
Does this seem like a decent approach? Other suggestions? Pitfalls?
The text was updated successfully, but these errors were encountered: