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

xianjingfeng · 2024-07-30T01:56:59Z

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 · 2024-07-30T02:01:57Z

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.

jerqi · 2024-07-30T02:31:21Z

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

xianjingfeng · 2024-07-30T03:01:31Z

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 · 2024-07-30T06:04:57Z

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

advancedxy · 2024-07-31T03:00:07Z

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 · 2024-07-31T06:32:07Z

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.

When using JDK8, compiling with maven.compiler.release will fail, so i think it not a good idea to update the pom.xml.
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.
If it influence the performance of the code, i think we can just update publish_maven_artifacts.sh
@advancedxy @summaryzb @jerqi @kaijchen

advancedxy · 2024-07-31T11:21:07Z

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 · 2024-07-31T13:26:11Z

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 · 2024-08-01T15:46:40Z

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 · 2024-08-02T01:36:53Z

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 · 2024-08-02T02:24:07Z

Right.

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

xianjingfeng · 2024-08-02T06:52:02Z

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 · 2024-08-02T12:08:00Z

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 · 2024-08-02T12:29:29Z

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 · 2024-08-06T11:07:15Z

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

jerqi · 2024-08-06T11:10:44Z

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 · 2024-08-07T03:21:06Z

#1902 (comment)

BTW, this should be put into 0.9.1, too.

@jerqi @advancedxy

jerqi · 2024-08-07T03:25:27Z

#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 · 2024-08-07T03:39:18Z

#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 · 2024-08-07T09:36:32Z

pom.xml

+        <jdk>[9,)</jdk>
+      </activation>
+      <properties>
+        <maven.compiler.release>8</maven.compiler.release>


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?

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

advancedxy

Left one minor comment. LGTM otherwise.

advancedxy · 2024-08-08T03:30:57Z

@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 · 2024-08-08T07:12:11Z

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 · 2024-08-09T02:22:28Z

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 · 2024-08-09T02:29:00Z

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

…greater than 8 (#1983) ### What changes were proposed in this pull request? Set maven.compiler.release to 8 by default when compiling with higher JDK versions ### Why are the changes needed? Fix: #1982 ### Does this PR introduce _any_ user-facing change? Yes, kind of. Uniffle client built by higher JDK versions can would directly on JDK 1.8 ### How was this patch tested? Compile with JDK11 and examine the byte code using `javap`. (cherry picked from commit cd61112)

…rsion greater than 8 (apache#1983) ### What changes were proposed in this pull request? Set maven.compiler.release to 8 by default when compiling with higher JDK versions ### Why are the changes needed? Fix: apache#1982 ### Does this PR introduce _any_ user-facing change? Yes, kind of. Uniffle client built by higher JDK versions can would directly on JDK 1.8 ### How was this patch tested? Compile with JDK11 and examine the byte code using `javap`. (cherry picked from commit cd61112)

specify maven.compiler.release in your pom.xml

b1654d1

specify maven.compiler.release in mvnw

b3b7d91

xianjingfeng changed the title ~~[#1982] fix(build): specify maven.compiler.release in pom.xml~~ [#1982] fix(build): specify maven.compiler.release while JDK version greater than 8 Jul 30, 2024

xianjingfeng requested a review from jerqi July 30, 2024 02:30

SteNicholas mentioned this pull request Jul 30, 2024

[GLUTEN-6483] Support Uniffle 0.9.0 apache/incubator-gluten#6484

Merged

nit

c4d15c7

advancedxy reviewed Aug 7, 2024

View reviewed changes

advancedxy previously approved these changes Aug 7, 2024

View reviewed changes

add docs

6f5c6c9

xianjingfeng dismissed advancedxy’s stale review via 6f5c6c9 August 8, 2024 02:58

advancedxy approved these changes Aug 8, 2024

View reviewed changes

advancedxy merged commit cd61112 into apache:master Aug 9, 2024

xianjingfeng deleted the issue_1982 branch August 9, 2024 02:29

xianjingfeng mentioned this pull request Aug 22, 2024

[#2077] improvement: Check java version in release scripts #2078

Closed

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

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

Uh oh!

Conversation

xianjingfeng commented Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

github-actions bot commented Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

jerqi commented Jul 30, 2024

Uh oh!

xianjingfeng commented Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

summaryzb commented Jul 30, 2024

Uh oh!

advancedxy commented Jul 31, 2024

Uh oh!

xianjingfeng commented Jul 31, 2024

Uh oh!

advancedxy commented Jul 31, 2024

Uh oh!

xianjingfeng commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

advancedxy commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xianjingfeng commented Aug 2, 2024

Uh oh!

advancedxy commented Aug 2, 2024

Uh oh!

xianjingfeng commented Aug 2, 2024

Uh oh!

advancedxy commented Aug 2, 2024

Uh oh!

xianjingfeng commented Aug 2, 2024

Uh oh!

advancedxy commented Aug 6, 2024

Uh oh!

jerqi commented Aug 6, 2024

Uh oh!

rickyma commented Aug 7, 2024

Uh oh!

jerqi commented Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rickyma commented Aug 7, 2024

Uh oh!

advancedxy Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

advancedxy Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

xianjingfeng Aug 8, 2024

Choose a reason for hiding this comment

Uh oh!

advancedxy left a comment

Choose a reason for hiding this comment

Uh oh!

advancedxy commented Aug 8, 2024

Uh oh!

advancedxy commented Aug 8, 2024

Uh oh!

advancedxy commented Aug 9, 2024

Uh oh!

xianjingfeng commented Aug 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

xianjingfeng commented Jul 30, 2024 •

edited

Loading

github-actions bot commented Jul 30, 2024 •

edited

Loading

xianjingfeng commented Jul 30, 2024 •

edited

Loading

xianjingfeng commented Jul 31, 2024 •

edited

Loading

advancedxy commented Aug 1, 2024 •

edited

Loading

jerqi commented Aug 7, 2024 •

edited

Loading