Skip to content

[GLUTEN-10570][FLINK] Add --add-opens options to MAVEN_OPTS for Java 17 compatibility#10572

Merged
philo-he merged 5 commits intoapache:mainfrom
KevinyhZou:fix_ut_failed_on_jdk17
Sep 4, 2025
Merged

[GLUTEN-10570][FLINK] Add --add-opens options to MAVEN_OPTS for Java 17 compatibility#10572
philo-he merged 5 commits intoapache:mainfrom
KevinyhZou:fix_ut_failed_on_jdk17

Conversation

@KevinyhZou
Copy link
Copy Markdown
Contributor

@KevinyhZou KevinyhZou commented Aug 28, 2025

What changes are proposed in this pull request?

fix flink ut failure on jdk17, related issue: #10570

How was this patch tested?

UT

@github-actions github-actions bot added the FLINK label Aug 28, 2025
@github-actions
Copy link
Copy Markdown

#10570

@philo-he
Copy link
Copy Markdown
Member

Flink CI is using JDK 17. I'm curious why this issue is not reported by CI.

@philo-he philo-he changed the title [GLUTEN-10570][FLINK]Fix UT failure on jdk17 [GLUTEN-10570][FLINK] Fix UT failure on jdk17 Aug 28, 2025
@philo-he philo-he changed the title [GLUTEN-10570][FLINK] Fix UT failure on jdk17 [GLUTEN-10570][FLINK] Fix UT failure on JDK 17 Aug 28, 2025
@jinchengchenghh
Copy link
Copy Markdown
Contributor

Maybe you also need, otherwise you may meet exception in the future, not sure for that.

      '--add-opens=java.base/java.lang=ALL-UNNAMED' \
      '--add-opens=java.base/java.lang.invoke=ALL-UNNAMED' \
      '--add-opens=java.base/java.lang.reflect=ALL-UNNAMED' \
      '--add-opens=java.base/java.io=ALL-UNNAMED' \
      '--add-opens=java.base/java.net=ALL-UNNAMED' \
      '--add-opens=java.base/java.nio=ALL-UNNAMED' \
      '--add-opens=java.base/java.util=ALL-UNNAMED' \
      '--add-opens=java.base/java.util.concurrent=ALL-UNNAMED' \
      '--add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED' \
      '--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED' \
      '--add-opens=java.base/sun.nio.ch=ALL-UNNAMED' \
      '--add-opens=java.base/sun.nio.cs=ALL-UNNAMED' \
      '--add-opens=java.base/sun.security.action=ALL-UNNAMED' \
      '--add-opens=java.base/sun.util.calendar=ALL-UNNAMED' \

@philo-he
Copy link
Copy Markdown
Member

@jinchengchenghh, I just became aware of this. The current Flink CI has already added some Maven arguments (including the proposed args of this PR) that ensure compilation and tests pass on JDK 17. See the link below.

@KevinyhZou, could you help confirm whether these remaining arguments also need to be added? @jinchengchenghh has provided a more comprehensive list, some of which are not necessary now but may become necessary in the future. Perhaps we should also consider adding these.

https://github.com/apache/incubator-gluten/blob/05c8703dd3473f4008bbc0196a7b083cc032ee17/.github/workflows/flink.yml#L25-L29

@philo-he philo-he closed this Aug 29, 2025
@philo-he philo-he reopened this Aug 29, 2025
@KevinyhZou
Copy link
Copy Markdown
Contributor Author

KevinyhZou commented Sep 1, 2025

@jinchengchenghh, I just became aware of this. The current Flink CI has already added some Maven arguments (including the proposed args of this PR) that ensure compilation and tests pass on JDK 17. See the link below.

@KevinyhZou, could you help confirm whether these remaining arguments also need to be added? @jinchengchenghh has provided a more comprehensive list, some of which are not necessary now but may become necessary in the future. Perhaps we should also consider adding these.

https://github.com/apache/incubator-gluten/blob/05c8703dd3473f4008bbc0196a7b083cc032ee17/.github/workflows/flink.yml#L25-L29

As java17 is still a experimental feature on flink-1.19: https://nightlies.apache.org/flink/flink-docs-release-1.19/docs/deployment/java_compatibility/#java-17, I think we'd better add these options to avoid the incompatibility on flink,or we should use jdk11 @philo-he

@jinchengchenghh
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot added INFRA and removed FLINK labels Sep 4, 2025
@KevinyhZou
Copy link
Copy Markdown
Contributor Author

I have added some possible options in flink.yml, please help to review it. @philo-he @jinchengchenghh

@philo-he
Copy link
Copy Markdown
Member

philo-he commented Sep 4, 2025

@KevinyhZou, thanks for your update. Do we need to add these args in gluten-flink/ut/pom.xml, similar to the following? Assume they are required for developers to run mvn test with Java 17 in the local.

https://github.com/apache/incubator-gluten/blob/7f5fb2d8a98d1410072858244235363530868593/pom.xml#L1622

@github-actions github-actions bot added the FLINK label Sep 4, 2025
@KevinyhZou
Copy link
Copy Markdown
Contributor Author

@KevinyhZou, thanks for your update. Do we need to add these args in gluten-flink/ut/pom.xml, similar to the following? Assume they are required for developers to run mvn test with Java 17 in the local.

https://github.com/apache/incubator-gluten/blob/7f5fb2d8a98d1410072858244235363530868593/pom.xml#L1622

done

Copy link
Copy Markdown
Member

@philo-he philo-he left a comment

Choose a reason for hiding this comment

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

Thanks.

@philo-he philo-he changed the title [GLUTEN-10570][FLINK] Fix UT failure on JDK 17 [GLUTEN-10570][FLINK] Add --add-opens options to MAVEN_OPTS for Java 17 compatibility Sep 4, 2025
@philo-he philo-he merged commit 3fe42d1 into apache:main Sep 4, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants