smiklosovic commented on code in PR #2046:
URL: https://github.com/apache/cassandra/pull/2046#discussion_r1055260105
##########
test/distributed/org/apache/cassandra/distributed/fuzz/HarryHelper.java:
##########
@@ -26,19 +26,26 @@
import harry.model.clock.OffsetClock;
import harry.model.sut.PrintlnSut;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.CASSANDRA_ALLOW_SIMPLESTRATEGY;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.CASSANDRA_MINIMUM_REPLICATION_FACTOR;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.DISABLE_TCACTIVE_OPENSSL;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.LOG4J2_DISABLE_JMX;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.LOG4J2_SHUTDOWNHOOKENABLED;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.ORG_APACHE_CASSANDRA_DISABLE_MBEAN_REGISTRATION;
+
public class HarryHelper
{
public static void init()
{
- System.setProperty("log4j2.disableJmx", "true"); // setting both ways
as changes between versions
Review Comment:
why was this removed? I know it looks super weird but I would keep it as it
was.
##########
test/distributed/org/apache/cassandra/distributed/impl/Instance.java:
##########
@@ -217,8 +222,8 @@ public LogAction logs()
{
// the path used is defined by test/conf/logback-dtest.xml and looks
like the following
//
./build/test/logs/${cassandra.testtag}/${suitename}/${cluster_id}/${instance_id}/system.log
- String tag = System.getProperty("cassandra.testtag",
"cassandra.testtag_IS_UNDEFINED");
- String suite = System.getProperty("suitename",
"suitename_IS_UNDEFINED");
+ String tag =
TEST_CASSANDRA_TESTTAG.getString("cassandra.testtag_IS_UNDEFINED");
Review Comment:
these can be specified like defaults maybe?
##########
test/unit/org/apache/cassandra/CassandraXMLJUnitResultFormatter.java:
##########
@@ -73,17 +75,17 @@ private static DocumentBuilder getDocumentBuilder() {
}
}
- private static final String tag = System.getProperty("cassandra.testtag",
"");
+ private static final String tag = TEST_CASSANDRA_TESTTAG.getString();
/*
* Set the property for the test suite name so that log configuration can
pick it up
* and log to a file specific to this test suite
*/
static
{
- String command = System.getProperty("sun.java.command");
+ String command = System.getProperty("sun.java.command"); //
checkstyle: suppress nearby 'blockSystemPropertyUsage'
Review Comment:
why this can't be replaced by CassandraRelevantProperties?
##########
test/distributed/org/apache/cassandra/distributed/shared/ClusterUtils.java:
##########
@@ -255,13 +258,13 @@ public static <I extends IInstance> I
replaceHostAndStart(AbstractCluster<I> clu
return start(inst, properties -> {
// lower this so the replacement waits less time
- properties.setProperty("cassandra.broadcast_interval_ms",
Long.toString(TimeUnit.SECONDS.toMillis(30)));
+ properties.set(BROADCAST_INTERVAL_MS,
Long.toString(TimeUnit.SECONDS.toMillis(30)));
// default is 30s, lowering as it should be faster
- properties.setProperty("cassandra.ring_delay_ms",
Long.toString(TimeUnit.SECONDS.toMillis(10)));
+ properties.set(RING_DELAY,
Long.toString(TimeUnit.SECONDS.toMillis(10)));
properties.set(BOOTSTRAP_SCHEMA_DELAY_MS,
TimeUnit.SECONDS.toMillis(10));
// state which node to replace
- properties.setProperty("cassandra.replace_address_first_boot",
toReplace.config().broadcastAddress().getAddress().getHostAddress());
+
REPLACE_ADDRESS_FIRST_BOOT.setString(toReplace.config().broadcastAddress().getAddress().getHostAddress());
Review Comment:
should not this be `properties.set(REPLACE_ADDRESS_FIRST_BOOT, ...` ?
##########
test/unit/org/apache/cassandra/SchemaLoader.java:
##########
@@ -757,7 +760,7 @@ public static void cleanupSavedCaches()
private static CompressionParams compressionParams(int chunkLength)
{
- String algo = System.getProperty("cassandra.test.compression.algo",
"lz4").toLowerCase();
+ String algo = TEST_COMPRESSION_ALGO.getString("lz4").toLowerCase();
Review Comment:
lz4 can be made default value in enum probably?
##########
test/unit/org/apache/cassandra/config/CassandraRelevantPropertiesTest.java:
##########
@@ -32,12 +32,12 @@ public void testString()
{
try
{
- System.setProperty(TEST_PROP.getKey(), "some-string");
Review Comment:
This is interesting change. We were testing that setting system property
will resolve that property from enum. But if we change the test that we set
that value via CassandraRelevantProperty, where is the guarantee that we have
set the _system property_ ? The test was made with some purpose / intent in
mind which I think was correct.
If we change this, then there needs to be a test which tests that if we set
a property via enum, then we can read it when we use `System.getProperty`.
##########
test/distributed/org/apache/cassandra/distributed/test/IPMembershipTest.java:
##########
@@ -64,7 +65,7 @@ public void sameIPFailWithoutReplace() throws IOException
nodeToReplace.config().set("auto_bootstrap", auto_bootstrap);
Assertions.assertThatThrownBy(() -> nodeToReplace.startup())
- .hasMessage("A node with address /127.0.0.3:7012
already exists, cancelling join. Use cassandra.replace_address if you want to
replace this node.");
+ .hasMessage("A node with address /127.0.0.3:7012
already exists, cancelling join. Use " + REPLACE_ADDRESS.getKey() + " if you
want to replace this node.");
Review Comment:
spice it up with -D and "property if you want ..."
##########
test/distributed/org/apache/cassandra/distributed/test/UpdateSystemAuthAfterDCExpansionTest.java:
##########
@@ -113,7 +115,7 @@ static public void beforeClass() throws Throwable
{
// reduce the time from 10s to prevent "Cannot process role related
query as the role manager isn't yet setup."
// exception from CassandraRoleManager
- System.setProperty("cassandra.superuser_setup_delay_ms", "0");
+ CassandraRelevantProperties.SUPERUSER_SETUP_DELAY_MS.setLong(0);
Review Comment:
setInt is probably equally good
##########
test/distributed/org/apache/cassandra/distributed/test/AlterTest.java:
##########
@@ -98,7 +99,7 @@ public void
alteringKeyspaceOnInsufficientNumberOfReplicasFiltersOutGossppingOnl
{
IInstanceConfig config = cluster.newInstanceConfig();
IInvokableInstance gossippingOnlyMember =
cluster.bootstrap(config);
- withProperty("cassandra.join_ring", Boolean.toString(false), () ->
gossippingOnlyMember.startup(cluster));
+ withProperty(CassandraRelevantProperties.JOIN_RING, false, () ->
gossippingOnlyMember.startup(cluster));
Review Comment:
can be static import
##########
test/unit/org/apache/cassandra/config/DatabaseDescriptorTest.java:
##########
@@ -504,7 +500,7 @@ public void testConcurrentValidations()
catch (ConfigurationException e)
{
assertThat(e.getMessage()).isEqualTo("To set
concurrent_validations > concurrent_compactors, " +
- "set the system property
cassandra.allow_unlimited_concurrent_validations=true");
+ "set the system property " +
ALLOW_UNLIMITED_CONCURRENT_VALIDATIONS.getKey() + "=true");
Review Comment:
-D
##########
test/distributed/org/apache/cassandra/distributed/fuzz/HarryHelper.java:
##########
@@ -26,19 +26,26 @@
import harry.model.clock.OffsetClock;
import harry.model.sut.PrintlnSut;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.CASSANDRA_ALLOW_SIMPLESTRATEGY;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.CASSANDRA_MINIMUM_REPLICATION_FACTOR;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.DISABLE_TCACTIVE_OPENSSL;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.LOG4J2_DISABLE_JMX;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.LOG4J2_SHUTDOWNHOOKENABLED;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.ORG_APACHE_CASSANDRA_DISABLE_MBEAN_REGISTRATION;
+
public class HarryHelper
{
public static void init()
{
- System.setProperty("log4j2.disableJmx", "true"); // setting both ways
as changes between versions
- System.setProperty("log4j2.disable.jmx", "true");
- System.setProperty("log4j.shutdownHookEnabled", "false");
- System.setProperty("cassandra.allow_simplestrategy", "true"); // makes
easier to share OSS tests without RF limits
- System.setProperty("cassandra.minimum_replication_factor", "0"); //
makes easier to share OSS tests without RF limits
-
- System.setProperty("cassandra.disable_tcactive_openssl", "true");
- System.setProperty("relocated.shaded.io.netty.transport.noNative",
"true");
- System.setProperty("org.apache.cassandra.disable_mbean_registration",
"true");
+ // setting both ways as changes between versions
+ System.setProperty("log4j2.disableJmx", "true"); // checkstyle:
suppress nearby 'blockSystemPropertyUsage'
+ LOG4J2_DISABLE_JMX.setBoolean(true);
+ LOG4J2_SHUTDOWNHOOKENABLED.setBoolean(false);
Review Comment:
you changed `log4j.shutdownHookEnabled` to `log4j2.shutdownHookEnabled`
--
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]
