GG-36475 Fix the illegal reflective access warnings issue #3070

dy2285 · 2024-02-01T23:21:07Z

By adding some JVM parameters in bin/control.sh, the issue of "illegal reflective access warnings" was addressed.

iakkuratov · 2024-02-02T09:34:38Z

Take a look at the file /bin/include/jvmdefaults.sh. I think most of the options have to be moved there

dy2285 · 2024-03-04T05:07:49Z

@iakkuratov added the jvm options to jvmdefaults.sh and corresponding changes in control.sh.

dy2285 · 2024-03-04T05:27:47Z

@iakkuratov please review the changes.

dy2285 · 2024-03-04T22:02:54Z

Undid the commits that added a wrong file name. Cleaned up the empty lines.

iakkuratov · 2024-03-05T07:04:02Z

bin/controlsh

This file should not exist.

iakkuratov · 2024-03-05T07:10:32Z

bin/control.sh

+CONTROL_JVM_OPTS="${CONTROL_JVM_OPTS} --add-exports=java.base/jdk.internal.misc=ALL-UNNAMED"
+CONTROL_JVM_OPTS="${CONTROL_JVM_OPTS} --add-exports=java.base/sun.nio.ch=ALL-UNNAMED"
+CONTROL_JVM_OPTS="${CONTROL_JVM_OPTS} --add-opens=java.base/jdk.internal.misc=ALL-UNNAMED"
+CONTROL_JVM_OPTS="${CONTROL_JVM_OPTS} --add-opens=java.base/sun.nio.ch=ALL-UNNAMED"
+CONTROL_JVM_OPTS="${CONTROL_JVM_OPTS} --add-opens=java.base/sun.nio.ch=ALL-UNNAMED"
+CONTROL_JVM_OPTS="${CONTROL_JVM_OPTS} --add-opens=java.management/com.sun.jmx.mbeanserver=ALL-UNNAMED"
+CONTROL_JVM_OPTS="${CONTROL_JVM_OPTS} --add-opens=jdk.internal.jvmstat/sun.jvmstat.monitor=ALL-UNNAMED"
+CONTROL_JVM_OPTS="${CONTROL_JVM_OPTS} --add-opens=java.base/sun.reflect.generics.reflectiveObjects=ALL-UNNAMED"
+CONTROL_JVM_OPTS="${CONTROL_JVM_OPTS} --add-opens=jdk.management/com.sun.management.internal=ALL-UNNAMED"
+CONTROL_JVM_OPTS="${CONTROL_JVM_OPTS} --add-opens=java.base/java.io=ALL-UNNAMED"
+CONTROL_JVM_OPTS="${CONTROL_JVM_OPTS} --add-opens=java.base/java.nio=ALL-UNNAMED"
+CONTROL_JVM_OPTS="${CONTROL_JVM_OPTS} --add-opens=java.base/java.util=ALL-UNNAMED"
+CONTROL_JVM_OPTS="${CONTROL_JVM_OPTS} --add-opens=java.base/java.lang=ALL-UNNAMED"
+CONTROL_JVM_OPTS="${CONTROL_JVM_OPTS} --illegal-access=warn"


If I get it right this changes have to be reverted since you moved options to a separate file

iakkuratov

Looks good to me. Please double-check spaces before strings and feel free to proceed with the core review.

sk0x50 · 2024-04-22T10:31:25Z

bin/control.sh

@@ -40,12 +40,19 @@ fi
 # Set SCRIPTS_HOME - base path to scripts.
 #
 SCRIPTS_HOME="${IGNITE_HOME_TMP}/bin"
+. "${SCRIPTS_HOME}"/include/functions.sh


I am a bit confused about this change. IMHO, we need to add additional JVM options to the CONTROL_JVM_OPTS variable. Something like as follows:

CONTROL_JVM_OPTS=$(getJavaSpecificOptsForControlScript $version "$CONTROL_JVM_OPTS")

where getJavaSpecificOptsForControlScript should determine the required options based on the JVM version like the getJavaSpecificOpts does.

By the way, this fix should be applied to the control.bat script as well. Perhaps, it makes sense to update snapshot-utility scripts in the same way. WDYT?

I have updated control.sh to use getJavaSpecificOpts for handling Java version-specific options. Based on your suggestion, are there additional requirements or different needs for control.sh that require a separate function? If getJavaSpecificOpts isn't suitable for this script, I agree that creating a new function, getJavaSpecificOptsForControlScript, could ensure we handle all specific settings effectively. I'll proceed with this approach if it matches your observations. And yes the changes we make to control.sh should also be applied to control.bat and potentially the snapshot-utility scripts to ensure uniform behavior across our toolset.

If I got your point right, you propose updating getJavaSpecificOpts instead of introducing a specific method for control.sh. Well, the idea looks good at first glance.

sk0x50 · 2024-04-22T10:32:32Z

bin/control.sh

@@ -112,7 +119,7 @@ fi
 # Assertions are disabled by default since version 3.5.
 # If you want to enable them - set 'ENABLE_ASSERTIONS' flag to '1'.
 #
-ENABLE_ASSERTIONS="1"
+ENABLE_ASSERTIONS="0"


sk0x50 · 2024-04-22T10:34:41Z

bin/control.sh

-         -DIGNITE_UPDATE_NOTIFIER=false -DIGNITE_HOME="${IGNITE_HOME}" \
-         $(getIbmSslOpts $version) \
+          -DIGNITE_HOME="${IGNITE_HOME}" \
+         -Dcom.ibm.jsse2.overrideDefaultTLS=true -Dssl.KeyManagerFactory.algorithm=IbmX509 \


Could you please clarify why this change is needed? I would say that using $(getIbmSslOpts $version) is the right way.

You are right that maintaining dynamic handling of SSL options through getIbmSslOpts is more flexible and maintainable, especially as it adapts to different Java versions. The hardcoded SSL options were used during simplified testing, which I should have clarified. I will revert this change to restore the use of getIbmSslOpts to ensure that our configuration remains robust and adaptable to various environments.

sk0x50 · 2024-04-22T10:37:06Z

bin/include/jvmdefaults.sh

@@ -103,3 +103,9 @@ getJavaSpecificOpts() {

  echo $value
 }
+# Reflective access options
+addReflectiveAccessOptions() { 


I think the exact list of options should be based on the JVM version. For example, using these opts with JVM8 leads to

Unrecognized option: --add-exports=java.base/jdk.internal.misc=ALL-UNNAMED Error: Could not create the Java Virtual Machine. Error: A fatal exception has occurred. Program will exit.

Perhaps, I am missing something.

It would be nice to check the script works as expected using Java 8, 11, and 17
https://www.gridgain.com/docs/latest/getting-started/quick-start/java

Thank you for highlighting the compatibility issues with Java 8. I realize now that my updates to the jvmdefaults.sh script included Java 9+ specific options without checks for Java version compatibility. I will revise the script to dynamically adjust the JVM options based on the version it detects.

Yes as recommended I will also conduct testing across these Java versions to confirm that the script functions correctly in each environment.

Once you've confirmed my approach regarding the initial comment of the PR, I will make all necessary changes and conduct all tests at once.

…ibility and address warnings

dy2285 · 2024-05-14T12:12:38Z

Made changes based on above discussion. Included control.bat and 'jvmdefaults.bat' . Will make a separate pr for snapshot-utility scripts. Please review when you have time. Thank you.

gg-36475 Fix the illegal reflective access warnings issue

2543845

dy2285 requested a review from iakkuratov February 1, 2024 23:22

To correct the mistake of adding a wrong file controlsh

b7386ce

dy2285 force-pushed the gg-36475 branch from 6da7146 to b7386ce Compare March 4, 2024 21:58

iakkuratov requested changes Mar 5, 2024

View reviewed changes

dy2285 and others added 3 commits March 5, 2024 22:15

Remove unwanted file controlsh

3311898

Update bin/control.sh and bin/include/jvmdefaults.sh

6f3416b

Merge branch 'master' into gg-36475

51bb31e

iakkuratov approved these changes Mar 21, 2024

View reviewed changes

iakkuratov requested a review from sk0x50 April 17, 2024 16:31

sk0x50 requested changes Apr 22, 2024

View reviewed changes

sk0x50 changed the title ~~gg-36475 Fix the illegal reflective access warnings issue~~ GG-36475 Fix the illegal reflective access warnings issue Apr 22, 2024

Update control scripts and JVM defaults to handle Java version compat…

0f58a07

…ibility and address warnings

dy2285 requested a review from sk0x50 May 14, 2024 12:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GG-36475 Fix the illegal reflective access warnings issue #3070

GG-36475 Fix the illegal reflective access warnings issue #3070

dy2285 commented Feb 1, 2024

iakkuratov commented Feb 2, 2024

dy2285 commented Mar 4, 2024

dy2285 commented Mar 4, 2024

dy2285 commented Mar 4, 2024

iakkuratov Mar 5, 2024

dy2285 Mar 6, 2024

iakkuratov Mar 5, 2024

iakkuratov left a comment

sk0x50 Apr 22, 2024

dy2285 May 8, 2024 •

edited

sk0x50 May 10, 2024

sk0x50 Apr 22, 2024

sk0x50 Apr 22, 2024

dy2285 May 8, 2024

sk0x50 Apr 22, 2024 •

edited

dy2285 May 8, 2024

dy2285 commented May 14, 2024

GG-36475 Fix the illegal reflective access warnings issue #3070

Are you sure you want to change the base?

GG-36475 Fix the illegal reflective access warnings issue #3070

Conversation

dy2285 commented Feb 1, 2024

iakkuratov commented Feb 2, 2024

dy2285 commented Mar 4, 2024

dy2285 commented Mar 4, 2024

dy2285 commented Mar 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iakkuratov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dy2285 May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sk0x50 Apr 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dy2285 commented May 14, 2024

dy2285 May 8, 2024 •

edited

sk0x50 Apr 22, 2024 •

edited