Skip to content

[AMORO-4384]add jdk version check and add more add-opens - #4385

Merged
zhoujinsong merged 3 commits into
apache:masterfrom
Aireed:issue_4384
Sep 22, 2026
Merged

zhoujinsong merged 3 commits into
apache:masterfrom
Aireed:issue_4384

Conversation

@Aireed

@Aireed Aireed commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Why are the changes needed?

Close #4384 .

Brief change log

  • add java version check in load-config script and add more extra option dependes on the java version

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before making a pull request

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

@sivakumar-mahalingam

Copy link
Copy Markdown
Contributor

Could we use $JAVA_RUN -version instead of java -version? If PATH points to JDK 11 but JAVA_HOME points to JDK 17, the script detects Java 11 while AMS runs on Java 17. Consequently, the required --add-opens option is not added, leaving #4384 unresolved.

@Aireed

Aireed commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Could we use $JAVA_RUN -version instead of java -version? If PATH points to JDK 11 but JAVA_HOME points to JDK 17, the script detects Java 11 while AMS runs on Java 17. Consequently, the required --add-opens option is not added, leaving #4384 unresolved.

thanks for you advice. nice catch. i have fixed it. PTAL

@Aireed
Aireed requested a review from zhoujinsong September 22, 2026 02:58
@sivakumar-mahalingam

Copy link
Copy Markdown
Contributor

Could we use $JAVA_RUN -version instead of java -version? If PATH points to JDK 11 but JAVA_HOME points to JDK 17, the script detects Java 11 while AMS runs on Java 17. Consequently, the required --add-opens option is not added, leaving #4384 unresolved.

thanks for you advice. nice catch. i have fixed it. PTAL

You are welcome. The fix looks fine. @zhoujinsong please validate

@zhoujinsong zhoujinsong left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.

BTW, I noticed the optimizer.sh script have some similar logic about changing the JVM variables depend on the JDK version.
We may polish the implementation here in another PR.

@zhoujinsong
zhoujinsong merged commit ed4623f into apache:master Sep 22, 2026
1 check passed
czy006 pushed a commit that referenced this pull request Sep 23, 2026
* fix# 4384 add jdk version check and add more add-opens

* fix

* fix according to the comment
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.

[Bug]: [jdk17]java.security.jgss does not export sun.security.krb5 to unnamed module

3 participants