Upgrade to JDK17.

[VenkatasivareddyTR]

Issue #, if available:
#2295
Description of changes:
Upgrade to JDK 17.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[macohen]

Thanks @VenkatasivareddyTR; can you check on the build error, please:

Error:  Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.13.0:compile (default-compile) on project aws-athena-federation-sdk: Fatal error compiling: error: release version 17 not supported -> [Help 1]

https://github.com/awslabs/aws-athena-query-federation/actions/runs/11475342726/job/31973995753?pr=2342

[VenkatasivareddyTR]

@macohen The build process appears to be running on JDK 11. To align with the codebase, we need to update the build environment to use JDK 17.

[macohen]

the code looks fine, but the GH actions are breaking and won't work until we're building with JDK 17 it seems. Is that right? are you able to update the actions (I know this build still won't pass until those are merged)...

[abhishekpoddar-trianz]

Yes correct, we would need to update the GH actions with JDK 17 to build this PR successfully. We have tested this in our local environments with JDK 17 setup.

To update the GH actions to take JDK 17, we are updating the respective YAML files (in .github/workflows/). We will be testing it in a fork before raising the PR for the same.

[abhishekpoddar-trianz]

PR for GitHub Actions update for java 17.
#2377

[macohen]

@mschoeni1 can we merge this into a new branch and then merge master on top of that? I think we need to work on producing JDK 11 AND JDK 17 builds in GH actions before that gets merged back to master. What do you think? cc: @aimethed

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.68%. Comparing base (c83e8f2) to head (70871a6).
Report is 61 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2342      +/-   ##
============================================
- Coverage     61.21%   60.68%   -0.54%     
- Complexity     3756     3870     +114     
============================================
  Files           577      593      +16     
  Lines         21435    22130     +695     
  Branches       2659     2732      +73     
============================================
+ Hits          13122    13429     +307     
- Misses         7042     7398     +356     
- Partials       1271     1303      +32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mschoeni1]

Thanks @Jithendar12, this looks great. Could you please just also update the README to include the command for users to be able to build with JDK17?

[Jithendar12]

Thanks @Jithendar12, this looks great. Could you please just also update the README to include the command for users to be able to build with JDK17?

Hi @mschoeni1 ,

Thank you for reviewing the PR! I believe it would be better to create a separate wiki page for this and provide a reference to it in the main README.

Please find below sample wiki page that i have created in my fork and documentation for the content. Let us know your thoughts on this.Thank you!

https://github.com/Jithendar12/aws-athena-query-federation/wiki/How-To-Build-And-Deploy-A-Connector-Using-JDK-17

jdk17-connector-build.odt

[AbdulR3hman]

few questions;

1. regarding JAVA_TOOL_OPTIONS being used as ARG vs ENV; then we proceed by setting the empty ARG to the ENV; but the comment says we are setting it to JDK 17; I think I might be missing something. I assume from this comment [RFC] JDK 17 Upgrade & JDK LTS  #2295 (comment) it is coming from yaml; where exactly?

2. we're adding JDK 17 profiles to athena-federation-sdk/pom.xml | but at the same time we are adding both JDK8 and 17 to the main pom.xml; why the discrepancy?

3. Following up from that; if we added the profile in the main maven; profiles are not inherited according to maven docs; this is why I assume we are using activation in order to enable the profile on children POMs?

4. Same goes for the plugins; the parent pom file has maven-surefire-plugin introduced to it; why are we introducing the same code to all children? But the plugins are inherited by children POM; so why are we doing this?

5. Lastly; where are you plan to utilize the jdk 17 profile? that was introduced?

[Jithendar12]

few questions;

1. regarding `JAVA_TOOL_OPTIONS` being used as `ARG` vs `ENV`;  then we proceed by setting the empty ARG to the ENV; but the comment says we are setting it to JDK 17; I think I might be missing something. I assume from this comment [[RFC] JDK 17 Upgrade & JDK LTS  #2295 (comment)](https://github.com/awslabs/aws-athena-query-federation/issues/2295#issuecomment-2422484927) it is coming from yaml; where exactly?

2. we're adding JDK 17 profiles to `athena-federation-sdk/pom.xml` | but at the same time we are adding both JDK8 and 17 to the main `pom.xml`; why the discrepancy?

3. Following up from that; if we added the profile in the main maven; profiles are not inherited according to [maven docs](https://maven.apache.org/guides/introduction/introduction-to-profiles.html#Profile_Inheritance); this is why I assume we are using [activation](https://maven.apache.org/guides/introduction/introduction-to-profiles.html#JDK) in order to enable the profile on children POMs?

4. Same goes for the plugins; the parent pom file has `maven-surefire-plugin` introduced to it; why are we introducing the same code to all children? But the plugins are inherited by children POM; so why are we doing this?

5. Lastly; where are you plan to utilize the jdk 17 profile? that was introduced?

Hi @AbdulR3hman ,

1. We pass JAVA_TOOL_OPTIONS as a build argument (--build-arg) when building the Docker image. During the build process, the ARG is used to set the ENV variable in the image.
   Example command:
   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 .

2. The JDK 8 profile in the main pom.xml ensures that if a user is building the code with JDK 8 locally, then maven.compiler.release is overridden to 8. This avoids compatibility issues and ensures the correct compiler version is used.

3. Yes, you are correct. The activation ensures that the profiles are automatically enabled based on the detected JDK version. When the JDK version matches the activation criteria (e.g., jdk=17), the properties and plugins defined in the profile are applied automatically, even in child POMs.

4. Not all child pom, only few where we have explicility added this plugin instead of picking up from the main pom. This forced me to add profile again in those pom's.

5. The JDK 17 profile is used for setting maven.compiler.release to 17 and
   adding --add-opens=java.base/java.nio=ALL-UNNAMED option to the Surefire and Failsafe plugins. This configuration is required to ensure the unit and integration tests run successfully on JDK 17, as documented here.

Please refer to my wiki page on how we are building and deploying the connector using JDK 17. Thank you!

[AbdulR3hman]

Not all child pom, only few where we have explicility added this plugin instead of picking up from the main pom. This forced me to add profile again in those pom's.

Can't we remove this plugin from all children and reference it from the parent pom?

[AbdulR3hman]

I did further reading; I found this answer to a stackoverflow that we are attempting to replicate;

https://stackoverflow.com/a/53666166

The link above is to the answer not the main post; and explains how we could achieve children all inherting the plugin without having to be repetitive and add profiles or anything of that sort in the children pom. I wouldn't mind having this change as a separate PR; but could you at least experiment with it?

[AbdulR3hman]

last question I have; don't we need to make change to our github actions or is the intention to continue to use JDK11 by default? (question to the team as well as the PR's Author)

[Jithendar12]

Not all child pom, only few where we have explicility added this plugin instead of picking up from the main pom. This forced me to add profile again in those pom's.

Can't we remove this plugin from all children and reference it from the parent pom?

Hi @AbdulR3hman , this plugin has been removed from child pom's.

[Jithendar12]

I did further reading; I found this answer to a stackoverflow that we are attempting to replicate;

https://stackoverflow.com/a/53666166

The link above is to the answer not the main post; and explains how we could achieve children all inherting the plugin without having to be repetitive and add profiles or anything of that sort in the children pom. I wouldn't mind having this change as a separate PR; but could you at least experiment with it?

Thanks for the reference @AbdulR3hman. I have incorporated the changes in this PR only.

[abhishekpoddar-trianz]

last question I have; don't we need to make change to our github actions or is the intention to continue to use JDK11 by default? (question to the team as well as the PR's Author)

@AbdulR3hman - in case it's decided to move to JDK17 as default then for GH actions PR #2377 can be referred.