[#1982] fix(build): specify maven.compiler.release while JDK version greater than 8

[xianjingfeng]

What changes were proposed in this pull request?

Specify maven.compiler.release while JDK version greater than 8.

Why are the changes needed?

Fix: #1982

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Compile with JDK11 and examine the bytecode using javap.

[github-actions]

Test Results

 2 747 files  ±0   2 747 suites  ±0   5h 48m 4s ⏱️ + 9m 11s
   981 tests +2     980 ✅ +2   1 💤 ±0  0 ❌ ±0 
12 298 runs  +4  12 283 ✅ +4  15 💤 ±0  0 ❌ ±0 

Results for commit 6f5c6c9. ± Comparison against base commit 7c4be40.

This pull request removes 2 and adds 4 tests. Note that renamed tests count towards both.

org.apache.tez.dag.app.RssDAGAppMasterTest ‑ testFetchRemoteStorageFromCoordinator
org.apache.tez.dag.app.RssDAGAppMasterTest ‑ testFetchRemoteStorageFromDynamicConf

org.apache.tez.dag.app.RssDAGAppMasterTest ‑ testFetchRemoteStorageFromCoordinator{String}[1]
org.apache.tez.dag.app.RssDAGAppMasterTest ‑ testFetchRemoteStorageFromCoordinator{String}[2]
org.apache.tez.dag.app.RssDAGAppMasterTest ‑ testFetchRemoteStorageFromDynamicConf{String}[1]
org.apache.tez.dag.app.RssDAGAppMasterTest ‑ testFetchRemoteStorageFromDynamicConf{String}[2]

♻️ This comment has been updated with latest results.

[roryqi]

A concern. Will it influence the performance of the code?

[xianjingfeng]

A concern. Will it influence the performance of the code?

I'm not sure, I'm not familiar with this part. cc @EnricoMi @zuston @advancedxy @rickyma @kaijchen

[summaryzb]

Maybe it's better to add mvn options in here https://github.com/apache/incubator-gluten/pull/6484/files#diff-38eda6989d285e6a3d79a3a66cc656d0b6d483b3d5c77bd0969d878642fd0f0eL512

[advancedxy]

I don't think you should update the mvnw script. You should update the pom.xml in this repo or like @summaryzb suggested, compile uniffle with corresponding mvn options in gluten.

[xianjingfeng]

I don't think you should update the mvnw script. You should update the pom.xml in this repo or like @summaryzb suggested, compile uniffle with corresponding mvn options in gluten.

1. When using JDK8, compiling with maven.compiler.release will fail, so i think it not a good idea to update the pom.xml.
2. Compile uniffle with corresponding mvn options in gluten is not friendly to gluten users. If the users need to compile the Uniffle source code by himself, there is no need to publish the client package to the Maven repository.
3. If it influence the performance of the code, i think we can just update publish_maven_artifacts.sh
   @advancedxy @summaryzb @jerqi @kaijchen

[advancedxy]

When using JDK8, compiling with maven.compiler.release will fail, so i think it not a good idea to update the pom.xml.

Please refer this: https://maven.apache.org/plugins/maven-compiler-plugin/examples/set-compiler-release.html

Compile uniffle with corresponding mvn options in gluten is not friendly to gluten users. If the users need to compile the Uniffle source code by himself, there is no need to publish the client package to the Maven repository.

Can you elaborate a bit on the use case?

[xianjingfeng]

Can you elaborate a bit on the use case?

Currently, if users use the jar we published to the Maven repository, they must not use JDK8, otherwise they need to compile rss-client-spark3-shaded.jar themselves as Gluten ci does.
https://github.com/apache/incubator-gluten/blob/67d11f3c4abe81c58dc6debc80c15e79ace965dc/.github/workflows/velox_docker.yml#L507-L510

[advancedxy]

if users use the jar we published to the Maven repository, they must not use JDK8,

I'm not sure. I believe the published jar on maven is and should be compiled by JDK 1.8 when releasing. So they can be used under JDK 1.8?

[xianjingfeng]

if users use the jar we published to the Maven repository, they must not use JDK8,

I'm not sure. I believe the published jar on maven is and should be compiled by JDK 1.8 when releasing. So they can be used under JDK 1.8?

Right.

[advancedxy]

Right.

So why gluten need to compile uniffle themself if they are using JDK 1.8? Or there's other cases?

[xianjingfeng]

I believe the published jar on maven is and should be compiled by JDK 1.8 when releasing

I think I misread this. The 0.9.0 version of the jar currently released to the Maven repository is not compiled using JDK8, so gluten need to compile uniffle themself if they are using JDK 1.8.

[advancedxy]

I see, I did a check on the released jar of uniffle, it's indeed not compiled with JDK 1.8. I think we should fix that and maybe release a patch version of 0.9.1 to address this issue.

However, I don't think the fix should be made on the mvnw script, which is essentially just a wrapper file around mvn and should be rarely updated. Would you mind updating your code to use the conditional configuration in https://maven.apache.org/plugins/maven-compiler-plugin/examples/set-compiler-release.html instead?

[xianjingfeng]

I see, I did a check on the released jar of uniffle, it's indeed not compiled with JDK 1.8. I think we should fix that and maybe release a patch version of 0.9.1 to address this issue.

However, I don't think the fix should be made on the mvnw script, which is essentially just a wrapper file around mvn and should be rarely updated. Would you mind updating your code to use the conditional configuration in https://maven.apache.org/plugins/maven-compiler-plugin/examples/set-compiler-release.html instead?

OK for me. @jerqi WDYT

[advancedxy]

@jerqi do you have any comments about this? I think we should address this issue and release 0.9.1 quickly.

[roryqi]

I see, I did a check on the released jar of uniffle, it's indeed not compiled with JDK 1.8. I think we should fix that and maybe release a patch version of 0.9.1 to address this issue.
However, I don't think the fix should be made on the mvnw script, which is essentially just a wrapper file around mvn and should be rarely updated. Would you mind updating your code to use the conditional configuration in https://maven.apache.org/plugins/maven-compiler-plugin/examples/set-compiler-release.html instead?

OK for me. @jerqi WDYT

Ok for me, too.

[rickyma]

#1902 (comment)

BTW, this should be put into 0.9.1, too.

@jerqi @advancedxy

[roryqi]

#1902 (comment)

BTW, this should be put into 0.9.1, too.

@jerqi @advancedxy

+1, You can commit it to branch 0.9 directly.

[rickyma]

#1902 (comment)
BTW, this should be put into 0.9.1, too.
@jerqi @advancedxy

+1, You can commit it to branch 0.9 directly.

Done.

[advancedxy]

What if users want to build Uniffle with higher JDK and release with higher jdk versions?
Is it possible for them to run mvn with something like below?

./mvnw -Dmaven.compiler.release=11

If so, you should update the README.md about the Building Uniffle section as well?

[advancedxy]

BTW, I tested this PR locally, it works as expected.

[xianjingfeng]

Done

[advancedxy]

Left one minor comment. LGTM otherwise.

[advancedxy]

@zuston it seems the rust test is flaky, could you take a look at that when you have time: https://github.com/apache/incubator-uniffle/pull/1983/checks?check_run_id=28495024345

@xianjingfeng please re-trigger the failed CI.

[advancedxy]

I think this PR is ready to go. But let's wait some more time(until tomorrow or later tonight in Beijing time?) so that others can have a chance to review this one.

cc @zuston @EnricoMi @jerqi @kaijchen @rickyma @summaryzb.

[advancedxy]

Since there's no objections raised so for, I will merge this.

@xianjingfeng would you mind to send a PR to branch-0.9 to include this fix too?

[xianjingfeng]

Since there's no objections raised so for, I will merge this.

@xianjingfeng would you mind to send a PR to branch-0.9 to include this fix too?

ok