ekaterinadimitrova2 commented on code in PR #3696:
URL: https://github.com/apache/cassandra/pull/3696#discussion_r1909036265
##########
build.xml:
##########
@@ -307,6 +307,72 @@
<equals arg1="${ant.java.version}" arg2="17"/>
</condition>
+ <resources id="_jvm21_arg_items">
+ <string>-Djdk.attach.allowAttachSelf=true</string>
+
+ <string>-XX:+UseZGC</string>
+ <string>-XX:+ZGenerational</string>
+
+ <!-- Temporary workaround for jamm having incorrect default
CompressedOops for JDK21 -->
+ <string>-XX:-UseCompressedOops</string>
Review Comment:
What is the side effect? And what is the incorrect? Can we link a respective
issue, please?
##########
conf/jvm21-server.options:
##########
@@ -0,0 +1,108 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+###########################################################################
+# jvm21-server.options #
+# #
+# See jvm-server.options. This file is specific for Java 21 and newer. #
+###########################################################################
+
+#################
+# GC SETTINGS #
+#################
+
+
+### Generational ZGC settings
+-XX:+UseZGC
+-XX:+ZGenerational
+# Temporary workaround for jamm's incorrect default CompressedOops; if you're
using ZGC this needs to be set
+-XX:-UseCompressedOops
Review Comment:
Doesn't adding straight ZGC require ML consensus or something?
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -543,9 +543,12 @@ public static class SSTableConfig
public volatile DurationSpec.IntMinutesBound index_summary_resize_interval
= new DurationSpec.IntMinutesBound("60m");
@Replaces(oldName = "gc_log_threshold_in_ms", converter =
Converters.MILLIS_DURATION_INT, deprecated = true)
- public volatile DurationSpec.IntMillisecondsBound gc_log_threshold = new
DurationSpec.IntMillisecondsBound("200ms");
+ public volatile DurationSpec.IntMillisecondsBound gc_log_threshold = new
DurationSpec.IntMillisecondsBound("20s");
Review Comment:
Most people won't switch directly to JDK21 and ZGC when they upgrade.
Actually what people normally advise is separate upgrade path.
1) Upgrade to new C* major version on the common JDK
2) Upgrade your Java version
Also, I believe change of default GC will require some ML at least
notification, if not discussion and justification, more data.
##########
NEWS.txt:
##########
@@ -154,6 +154,11 @@ Upgrading
Deprecation
-----------
- `use_deterministic_table_id` is no longer supported and should be
removed from cassandra.yaml. Table IDs may still be supplied explicitly on
CREATE.
+ - Updating dependencies for JDK21 necessitated updating Chronicle Queue,
which has changed the enums used for log
Review Comment:
So we will opt in for deprecation in the next major and change on the one
after?
##########
redhat/cassandra.in.sh:
##########
@@ -121,46 +121,46 @@ if [ -z $JAVA ] ; then
exit 1;
fi
+# TODO: Factor this out to something we source so we don't have the
duplication all over our scripts
Review Comment:
Shall we even open a ticket? Or I think there might have been one Claude was
working on already
##########
conf/jvm-server.options:
##########
@@ -174,6 +174,9 @@
#-Xms4G
#-Xmx4G
+# Need experimental bytebuddy for JDK21
+-Dnet.bytebuddy.experimental
Review Comment:
Do we need it here? Do we use it in Python DTests? I don't think so.
##########
build.xml:
##########
@@ -307,6 +307,72 @@
<equals arg1="${ant.java.version}" arg2="17"/>
</condition>
+ <resources id="_jvm21_arg_items">
+ <string>-Djdk.attach.allowAttachSelf=true</string>
+
+ <string>-XX:+UseZGC</string>
+ <string>-XX:+ZGenerational</string>
+
+ <!-- Temporary workaround for jamm having incorrect default
CompressedOops for JDK21 -->
+ <string>-XX:-UseCompressedOops</string>
+
+ <!-- Have jamm tell us what it sees for memory layout on start for
debugging purposes -->
+ <string>-Dorg.github.jamm.strategies.LogInfoAtStartup-true</string>
Review Comment:
Do we want it popping up in all tests where jamm is used? Or we can add it
commented out? Maybe even add a note in `ObjectSizes` class? Same as we mention
there how to print trees
##########
conf/jvm21-clients.options:
##########
@@ -0,0 +1,53 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+###########################################################################
+# jvm21-clients.options #
+# #
+# See jvm-clients.options. This file is specific for Java 21 #
+###########################################################################
+
+###################
+# JPMS SETTINGS #
+###################
+
+-Djdk.attach.allowAttachSelf=true
+--add-exports java.base/jdk.internal.misc=ALL-UNNAMED
+--add-exports java.management.rmi/com.sun.jmx.remote.internal.rmi=ALL-UNNAMED
+--add-exports java.rmi/sun.rmi.registry=ALL-UNNAMED
+--add-exports java.rmi/sun.rmi.server=ALL-UNNAMED
+--add-exports java.sql/java.sql=ALL-UNNAMED
+--add-exports java.base/java.lang.ref=ALL-UNNAMED
+--add-exports jdk.unsupported/sun.misc=ALL-UNNAMED
+
+--add-opens java.base/java.lang.module=ALL-UNNAMED
+--add-opens java.base/jdk.internal.loader=ALL-UNNAMED
+--add-opens java.base/jdk.internal.ref=ALL-UNNAMED
+--add-opens java.base/jdk.internal.reflect=ALL-UNNAMED
+--add-opens java.base/jdk.internal.math=ALL-UNNAMED
+--add-opens java.base/jdk.internal.module=ALL-UNNAMED
+--add-opens java.base/jdk.internal.util.jar=ALL-UNNAMED
Review Comment:
Not needed in 21, it does not exist
##########
test/unit/org/apache/cassandra/tools/ToolRunner.java:
##########
@@ -57,6 +57,7 @@ public class ToolRunner
protected static final Logger logger =
LoggerFactory.getLogger(ToolRunner.class);
public static final ImmutableList<String> DEFAULT_CLEANERS =
ImmutableList.of("(?im)^picked up.*\\R",
+
"(?im)^.*package jdk.internal.util.jar not in java.base*\\R",
Review Comment:
You should just not add` jdk.internal.util.jar` in config for 21. It does
not exist there. If this warning still appears after the removal - then there
is a bug in what config we read
##########
test/unit/org/apache/cassandra/db/commitlog/CommitlogShutdownTest.java:
##########
@@ -58,7 +58,7 @@ public class CommitlogShutdownTest
@BMRule(name = "Make removing commitlog segments slow",
targetClass = "CommitLogSegment",
targetMethod = "discard",
- action = "Thread.sleep(50)")
+ action = "Thread.sleep(250L)")
Review Comment:
sleeps... :sigh
##########
conf/jvm21-server.options:
##########
@@ -0,0 +1,108 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+###########################################################################
+# jvm21-server.options #
+# #
+# See jvm-server.options. This file is specific for Java 21 and newer. #
+###########################################################################
+
+#################
+# GC SETTINGS #
+#################
+
+
+### Generational ZGC settings
+-XX:+UseZGC
+-XX:+ZGenerational
+# Temporary workaround for jamm's incorrect default CompressedOops; if you're
using ZGC this needs to be set
+-XX:-UseCompressedOops
+
+# Blanket enabling of all the THP / large pages available in the env
+# -XX:+UseLargePages
+# Max size ZGC will _attempt_ to stay below
+# -XX:+SoftMaxHeapSize=12G
+# Don't let ZGC return unclaimed memory to the OS by setting min and max ==
+# -Xms12G </string>
+# -Xmx12G </string>
+# Alternatively, can use this to disable returning memory to OS
+# -XX:-ZUncommit</string>
+# -XX:ZUncommitDelay=300</string>
+# -XX:+AlwaysPreTouch</string>
+
+### JPMS
+
+-Djdk.attach.allowAttachSelf=true
+--add-exports java.base/jdk.internal.misc=ALL-UNNAMED
+--add-exports java.management.rmi/com.sun.jmx.remote.internal.rmi=ALL-UNNAMED
+--add-exports java.rmi/sun.rmi.registry=ALL-UNNAMED
+--add-exports java.rmi/sun.rmi.server=ALL-UNNAMED
+--add-exports java.sql/java.sql=ALL-UNNAMED
+--add-exports java.base/java.lang.ref=ALL-UNNAMED
+--add-exports jdk.unsupported/sun.misc=ALL-UNNAMED
+
+--add-opens java.base/java.lang.module=ALL-UNNAMED
+--add-opens java.base/jdk.internal.loader=ALL-UNNAMED
+--add-opens java.base/jdk.internal.ref=ALL-UNNAMED
+--add-opens java.base/jdk.internal.reflect=ALL-UNNAMED
+--add-opens java.base/jdk.internal.math=ALL-UNNAMED
+--add-opens java.base/jdk.internal.module=ALL-UNNAMED
+--add-opens java.base/jdk.internal.util.jar=ALL-UNNAMED
Review Comment:
Not needed in 21, it does not exist
##########
src/java/org/apache/cassandra/utils/ObjectSizes.java:
##########
@@ -37,6 +37,11 @@
*/
public class ObjectSizes
{
+ /**
+ * For debugging purposes, you can construct this with .printVisitedTree()
to get a printout of all the objects
+ * and their calculated size on heap to debug jamm related issues. See:
+ * <a
href="https://github.com/jbellis/jamm/blob/master/README.md#visited-object-tree";>README.md
on github</a>
+ */
Review Comment:
See my build.xml comment, we can add a note here also about the other option
that prints useful info on start
##########
build.xml:
##########
@@ -45,7 +45,7 @@
The use of both CASSANDRA_USE_JDK11 and use-jdk11 is deprecated.
-->
<property name="java.default" value="11" />
- <property name="java.supported" value="11,17" />
+ <property name="java.supported" value="11,17,21" />
Review Comment:
I suggest until we can and start running CI with JDK21 we do not add 21 to
supported versions. BUT people can test with it by using the recently added
option - `CASSANDRA_JDK_UNSUPPORTED`,
[CASSANDRA-18688](https://issues.apache.org/jira/browse/CASSANDRA-18688). Once
we have CI in place - we can add it.
##########
conf/jvm21-clients.options:
##########
@@ -0,0 +1,53 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+###########################################################################
+# jvm21-clients.options #
+# #
+# See jvm-clients.options. This file is specific for Java 21 #
+###########################################################################
+
+###################
+# JPMS SETTINGS #
+###################
+
+-Djdk.attach.allowAttachSelf=true
Review Comment:
nit: it would be nice to sort alphabetically add-exports, add-opens. Makes
it easy not to duplicate things and see what is going on when the list becomes
too long.
##########
build.xml:
##########
@@ -307,6 +307,72 @@
<equals arg1="${ant.java.version}" arg2="17"/>
</condition>
+ <resources id="_jvm21_arg_items">
+ <string>-Djdk.attach.allowAttachSelf=true</string>
+
+ <string>-XX:+UseZGC</string>
+ <string>-XX:+ZGenerational</string>
+
+ <!-- Temporary workaround for jamm having incorrect default
CompressedOops for JDK21 -->
+ <string>-XX:-UseCompressedOops</string>
+
+ <!-- Have jamm tell us what it sees for memory layout on start for
debugging purposes -->
+ <string>-Dorg.github.jamm.strategies.LogInfoAtStartup-true</string>
+
+ <!-- <string>-XX:+UseLargePages</string> -->
+ <!-- <string>-XX:+SoftMaxHeapSize=12G</string> --> <!-- Max size ZGC
will _attempt_ to stay below -->
+ <!-- Disable returning memory to the OS -->
+ <string>-XX:-ZUncommit</string>
+ <!-- <string>-XX:ZUncommitDelay=300</string> -->
+
+ <!-- Pay the tax up front to pretouch memory to smooth out latency
later -->
+ <string>-XX:+AlwaysPreTouch</string>
+
+ <!-- Need to explicitly allow security manager on JDK21; deprecated
for removal -->
+ <string>-Djava.security.manager=allow</string>
+
+ <string>--add-exports java.base/jdk.internal.misc=ALL-UNNAMED</string>
+ <string>--add-exports java.base/jdk.internal.ref=ALL-UNNAMED</string>
+ <string>--add-exports java.base/sun.nio.ch=ALL-UNNAMED</string>
+ <string>--add-exports
java.management.rmi/com.sun.jmx.remote.internal.rmi=ALL-UNNAMED</string>
+
+ <string>--add-exports java.rmi/sun.rmi.registry=ALL-UNNAMED</string>
+ <string>--add-exports java.rmi/sun.rmi.server=ALL-UNNAMED</string>
+ <string>--add-exports
java.rmi/sun.rmi.transport.tcp=ALL-UNNAMED</string>
+ <string>--add-exports java.sql/java.sql=ALL-UNNAMED</string>
+ <string>--add-exports java.base/java.lang.ref=ALL-UNNAMED</string>
+ <string>--add-exports java.base/java.lang.reflect=ALL-UNNAMED</string>
+ <string>--add-exports jdk.unsupported/sun.misc=ALL-UNNAMED</string>
+ <string>--add-exports
jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED</string>
+
+ <string>--add-opens java.base/java.lang.module=ALL-UNNAMED</string>
+ <string>--add-opens java.base/java.net=ALL-UNNAMED</string>
+ <string>--add-opens java.base/jdk.internal.loader=ALL-UNNAMED</string>
+ <string>--add-opens java.base/jdk.internal.ref=ALL-UNNAMED</string>
+ <string>--add-opens java.base/jdk.internal.reflect=ALL-UNNAMED</string>
+ <string>--add-opens java.base/jdk.internal.math=ALL-UNNAMED</string>
+ <string>--add-opens java.base/jdk.internal.module=ALL-UNNAMED</string>
+ <string>--add-opens
java.base/jdk.internal.util.jar=ALL-UNNAMED</string>
Review Comment:
This line can be removed, it does not exist in 21. If you get an error with
21 about it that means there is some config error somewhere (which config files
we read, etc). It happened to me.
##########
src/java/org/apache/cassandra/db/commitlog/CommitLogSegment.java:
##########
@@ -638,6 +638,7 @@ public String dirtyString()
{
TableMetadata m = Schema.instance.getTableMetadata(tableId);
sb.append(m == null ? "<deleted>" : m.name).append("
(").append(tableId)
+ .append(", keyspace: ").append(m.keyspace)
Review Comment:
Same here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
