Skip to content

Conversation

@syuu1228
Copy link
Contributor

@syuu1228 syuu1228 commented Jul 4, 2024

Since we dropped Java check from unified installer and jmx, we need to do it on scylla-tools.

Related scylladb/scylladb#17969

@syuu1228 syuu1228 marked this pull request as ready for review July 4, 2024 23:38
@syuu1228
Copy link
Contributor Author

syuu1228 commented Jul 4, 2024

This is part of scylladb/scylladb#17969, need to merge first.

@denesb denesb requested a review from tchaikov July 8, 2024 06:51
done
}

# So far, scylla-jmx only works with Java 11 and Java 8, we prefer the newer
Copy link
Contributor

Choose a reason for hiding this comment

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

might want to s/scylla-jmx/scylla-tools/ . probably we could be more specific though, like "tools like nodetool and cassandra-stress". but "scylla-tools" is the name used by https://github.com/scylladb/scylla-tools-java?tab=readme-ov-file#scylla-tools .

Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

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

also, in scylla-jmx, select-java actually serves for use cases

in this change, we only address the second use case, where we are installing scylla-tools using install.sh, but what about the first use case? please note, the existing check is performed by

java_ver_output=`"${JAVA:-java}" -version 2>&1`
, which just check if the java fulfills the needs, it does not pick a java runtime. so if user actually has openjdk-11 installed, but he/she doe not point /usr/bin/java to it, or export $JAVA so that it points to one of the supported java runtime, cassandra.in.sh fails.

Since we dropped Java check from unified installer and jmx, we need to
do it on scylla-tools.

Related scylladb/scylladb#17969
@syuu1228 syuu1228 force-pushed the introduce_java_check branch from 38fd305 to 5281c88 Compare July 10, 2024 03:23
@syuu1228
Copy link
Contributor Author

also, in scylla-jmx, select-java actually serves for use cases

in this change, we only address the second use case, where we are installing scylla-tools using install.sh, but what about the first use case? please note, the existing check is performed by

java_ver_output=`"${JAVA:-java}" -version 2>&1`

, which just check if the java fulfills the needs, it does not pick a java runtime. so if user actually has openjdk-11 installed, but he/she doe not point /usr/bin/java to it, or export $JAVA so that it points to one of the supported java runtime, cassandra.in.sh fails.

We need some code to detect Java on scylla-tools's install.sh, since we are dropping it from unified installer.
But, as you descripbed, Java detection method between select-java and cassandra.in.sh are different, I think we should not use select-java here.
So I pushed new code to detect Java just like cassandra.in.sh, and dropped select-java.

fi

if ! builtin command -v $JAVA > /dev/null; then
echo "Please install openjdk-11 before running install.sh."
Copy link
Contributor

Choose a reason for hiding this comment

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

what if user installs, for instance, openjdk-21 and have it in $PATH? and we are not checking for openjdk-11, but a random java executable in the $PATH. so even if this test passes, it does not imply that we have openjdk-11 around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants