Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2543845
gg-36475 Fix the illegal reflective access warnings issue
dy2285 Feb 1, 2024
b7386ce
To correct the mistake of adding a wrong file controlsh
dy2285 Mar 4, 2024
3311898
Remove unwanted file controlsh
dy2285 Mar 6, 2024
6f3416b
Update bin/control.sh and bin/include/jvmdefaults.sh
dy2285 Mar 13, 2024
51bb31e
Merge branch 'master' into gg-36475
dy2285 Mar 13, 2024
0f58a07
Update control scripts and JVM defaults to handle Java version compat…
dy2285 May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 22 additions & 9 deletions bin/control.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,19 @@ fi
# Set SCRIPTS_HOME - base path to scripts.
#
SCRIPTS_HOME="${IGNITE_HOME_TMP}/bin"
. "${SCRIPTS_HOME}"/include/functions.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

@dy2285 dy2285 May 8, 2024 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


source "${SCRIPTS_HOME}"/include/functions.sh
source "${SCRIPTS_HOME}"/include/jvmdefaults.sh
# Include JVM options from jvmdefaults.sh
. "${SCRIPTS_HOME}"/include/jvmdefaults.sh ||exit 1

# Call the function
addReflectiveAccessOptions

# Use the function
CONTROL_JVM_OPTS="${CONTROL_JVM_OPTS} $(addReflectiveAccessOptions)"

#
# Discover path to Java executable and check it's version.
# Discover path to Java executable and check its version.
#
checkJava

Expand All @@ -62,7 +69,7 @@ fi
# Set IGNITE_LIBS.
#
. "${SCRIPTS_HOME}"/include/setenv.sh
. "${SCRIPTS_HOME}"/include/build-classpath.sh # Will be removed in the binary release.

CP="${IGNITE_LIBS}:${IGNITE_HOME}/libs/optional/ignite-zookeeper/*"

RANDOM_NUMBER=$("$JAVA" -cp "${CP}" org.apache.ignite.startup.cmdline.CommandLineRandomNumberGenerator)
Expand Down Expand Up @@ -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 marked this conversation as resolved.
Show resolved Hide resolved

#
# Set '-ea' options if assertions are enabled.
Expand All @@ -137,8 +144,14 @@ fi
#
# Final CONTROL_JVM_OPTS for Java 9+ compatibility
#



# Get Java specific options
CONTROL_JVM_OPTS=$(getJavaSpecificOpts $version "$CONTROL_JVM_OPTS")



if [ -n "${JVM_OPTS}" ] ; then
echo "JVM_OPTS environment variable is set, but will not be used. To pass JVM options use CONTROL_JVM_OPTS"
echo "JVM_OPTS=${JVM_OPTS}"
Expand All @@ -147,18 +160,18 @@ fi
case $osname in
Darwin*)
"$JAVA" ${CONTROL_JVM_OPTS} ${QUIET:-} "${DOCK_OPTS}" \
-DIGNITE_UPDATE_NOTIFIER=false -DIGNITE_HOME="${IGNITE_HOME}" \
-DIGNITE_HOME="${IGNITE_HOME}" \
-DIGNITE_PROG_NAME="$0" ${JVM_XOPTS:-} -cp "${CP}" ${MAIN_CLASS} "$@"
;;
OS/390*)
"$JAVA" ${CONTROL_JVM_OPTS} ${QUIET:-} \
-DIGNITE_UPDATE_NOTIFIER=false -DIGNITE_HOME="${IGNITE_HOME}" \
$(getIbmSslOpts $version) \
-DIGNITE_HOME="${IGNITE_HOME}" \
-Dcom.ibm.jsse2.overrideDefaultTLS=true -Dssl.KeyManagerFactory.algorithm=IbmX509 \
sk0x50 marked this conversation as resolved.
Show resolved Hide resolved
-DIGNITE_PROG_NAME="$0" ${JVM_XOPTS:-} -cp "${CP}" ${MAIN_CLASS} "$@"
;;
*)
"$JAVA" ${CONTROL_JVM_OPTS} ${QUIET:-} \
-DIGNITE_UPDATE_NOTIFIER=false -DIGNITE_HOME="${IGNITE_HOME}" \
-DIGNITE_HOME="${IGNITE_HOME}" \
-DIGNITE_PROG_NAME="$0" ${JVM_XOPTS:-} -cp "${CP}" ${MAIN_CLASS} "$@"
;;
esac
6 changes: 6 additions & 0 deletions bin/include/jvmdefaults.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,3 +103,9 @@ getJavaSpecificOpts() {

echo $value
}
# Reflective access options
addReflectiveAccessOptions() {
Copy link
Contributor

@sk0x50 sk0x50 Apr 22, 2024 •
edited

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

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

# Export the function
export -f addReflectiveAccessOptions