ekaterinadimitrova2 commented on code in PR #3696:
URL: https://github.com/apache/cassandra/pull/3696#discussion_r1929569695
##########
src/java/org/apache/cassandra/service/GCInspector.java:
##########
@@ -357,18 +405,63 @@ public void setGcLogThresholdInMs(long threshold)
if (gcWarnThresholdInMs != 0 && threshold > gcWarnThresholdInMs)
throw new IllegalArgumentException("Threshold must be less than
gcWarnThresholdInMs which is currently "
+ gcWarnThresholdInMs);
-
- DatabaseDescriptor.setGCLogThreshold((int) threshold);
+ DatabaseDescriptor.setZGCLogThreshold(threshold);
+ DatabaseDescriptor.setGCLogThreshold(threshold);
}
+ @Override
public long getGcLogThresholdInMs()
{
- return DatabaseDescriptor.getGCLogThreshold();
+ return hasZGC ? DatabaseDescriptor.getZGCLogThreshold() :
DatabaseDescriptor.getGCLogThreshold();
}
+ @Override
public long getStatusThresholdInMs()
{
return getGcWarnThresholdInMs() != 0 ? getGcWarnThresholdInMs() :
getGcLogThresholdInMs();
}
+ @Override
+ public void setGcPauseLogThresholdInMs(long threshold)
+ {
+ if (threshold <= 0)
Review Comment:
The `DurationSpec` class verifies only bigger than 0, and this method is
used only by JMX? If it is the source of truth, then I think we miss to check
for this setting not being set to 0 in cassandra.yaml. The check should be in
the DatabaseDescriptor, happening during startup.
##########
test/unit/org/apache/cassandra/config/LoadOldYAMLBackwardCompatibilityTest.java:
##########
@@ -100,8 +100,8 @@ public void testConfigurationLoaderBackwardCompatibility()
assertNull(config.networking_cache_size);
assertNull(config.file_cache_size);
assertNull(config.index_summary_capacity);
- assertEquals(new DurationSpec.IntMillisecondsBound(200),
config.gc_log_threshold);
- assertEquals(new DurationSpec.IntMillisecondsBound(1000),
config.gc_warn_threshold);
+ assertEquals(new DurationSpec.IntMillisecondsBound(20000),
config.gc_log_threshold);
+ assertEquals(new DurationSpec.IntMillisecondsBound(30000),
config.gc_warn_threshold);
assertEquals(new DurationSpec.IntSecondsBound(86400),
config.trace_type_query_ttl);
Review Comment:
Why this change if we reverted the defaults change? Wouldn't this test fail?
##########
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:
> That doesn't mean it has to be mandatory for pre-commit CI (so long as we
accept we can't yet blame folk for introducing jdk21 breakages). This aligns to
what I believe is being proposed here with prematurely introducing 21 without
dropping 11.
Fine, as long as we make it clear on the ML and people are ok to
retrospectively fix what they broke.
##########
test/unit/org/apache/cassandra/service/GCInspectorTest.java:
##########
@@ -66,12 +75,13 @@ public void ensureZeroIsOk()
Assert.assertEquals(gcInspector.getStatusThresholdInMs(),
gcInspector.getGcLogThresholdInMs());
Assert.assertEquals(0, DatabaseDescriptor.getGCWarnThreshold());
Assert.assertEquals(200, DatabaseDescriptor.getGCLogThreshold());
+ Assert.assertEquals(20000, DatabaseDescriptor.getZGCLogThreshold());
}
@Test(expected=IllegalArgumentException.class)
public void ensureLogLessThanWarn()
{
- Assert.assertEquals(200, gcInspector.getGcLogThresholdInMs());
+ Assert.assertEquals(20000, gcInspector.getGcLogThresholdInMs());
Review Comment:
Same question, about the change of default
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -4457,11 +4457,31 @@ public static long getGCLogThreshold()
return conf.gc_log_threshold.toMilliseconds();
}
- public static void setGCLogThreshold(int gcLogThreshold)
+ public static void setGCLogThreshold(long gcLogThreshold)
{
conf.gc_log_threshold = new
DurationSpec.IntMillisecondsBound(gcLogThreshold);
}
+ public static long getZGCLogThreshold()
+ {
+ return conf.gc_log_zgc_threshold.toMilliseconds();
Review Comment:
Not a blocker for this ticket and we do not have test coverage etc, neither
I think it is a blocker but as a future reminder - I recommend adding simple
unit tests to the `DatabaseDescriptorTest`.
##########
conf/jvm-server.options:
##########
@@ -174,6 +174,9 @@
#-Xms4G
#-Xmx4G
+# Need experimental bytebuddy for JDK21
+-Dnet.bytebuddy.experimental
Review Comment:
I am lost... I thought we removed and tested it is unnecessary, and we still
see it here. Please help me to understand what did I miss?
##########
NEWS.txt:
##########
@@ -71,6 +71,24 @@ using the provided 'sstableupgrade' tool.
New features
------------
+ - JDK21 and Generational ZGC is now officially supported. By default, when
JDK21 is used the server params in
Review Comment:
That sounds great, do we recommend this in prod already? I don't think I am
the right person to review the selected settings and say they are good for
prod.
--
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]
