From d22426298d17b81b46e199bd4d5abf0762e1347f Mon Sep 17 00:00:00 2001 From: David Capwell Date: Mon, 3 Feb 2025 15:24:43 -0800 Subject: [PATCH 1/2] CASSANDRA-20286: Accord: TableParamsTest is flakey due to bad generators and production validation logic missed the argument to String.format causing confusing errors --- .../ParameterizedFastPathStrategy.java | 2 +- .../cassandra/schema/TableParamsTest.java | 20 +++++++++---------- .../cassandra/utils/CassandraGenerators.java | 7 ++++++- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/src/java/org/apache/cassandra/service/accord/fastpath/ParameterizedFastPathStrategy.java b/src/java/org/apache/cassandra/service/accord/fastpath/ParameterizedFastPathStrategy.java index 8aef6abe6ab7..858c15b64ddc 100644 --- a/src/java/org/apache/cassandra/service/accord/fastpath/ParameterizedFastPathStrategy.java +++ b/src/java/org/apache/cassandra/service/accord/fastpath/ParameterizedFastPathStrategy.java @@ -146,7 +146,7 @@ static WeightedDc fromString(String s, int idx) else if (parts.length == 2) return new WeightedDc(validateDC(parts[0]), validateWeight(parts[1]), false); else - throw cfe("Invalid dc weighting syntax %s, use :"); + throw cfe("Invalid dc weighting syntax %s, use :", s); } } diff --git a/test/unit/org/apache/cassandra/schema/TableParamsTest.java b/test/unit/org/apache/cassandra/schema/TableParamsTest.java index e8bf30fe0aaa..9146264278f7 100644 --- a/test/unit/org/apache/cassandra/schema/TableParamsTest.java +++ b/test/unit/org/apache/cassandra/schema/TableParamsTest.java @@ -20,14 +20,14 @@ import org.junit.Test; +import accord.utils.Gen; import org.apache.cassandra.io.util.DataOutputBuffer; import org.apache.cassandra.tcm.membership.NodeVersion; import org.apache.cassandra.tcm.serialization.AsymmetricMetadataSerializers; import org.apache.cassandra.utils.CassandraGenerators.TableParamsBuilder; -import org.apache.cassandra.utils.FailingConsumer; -import org.quicktheories.core.Gen; +import org.apache.cassandra.utils.Generators; -import static org.quicktheories.QuickTheory.qt; +import static accord.utils.Property.qt; public class TableParamsTest @@ -36,17 +36,17 @@ public class TableParamsTest public void serdeLatest() { DataOutputBuffer output = new DataOutputBuffer(); - qt().forAll(tableParams()).checkAssert(FailingConsumer.orFail(params -> { + qt().forAll(tableParams()).check(params -> { AsymmetricMetadataSerializers.testSerde(output, TableParams.serializer, params, NodeVersion.CURRENT_METADATA_VERSION); - })); + }); } private static Gen tableParams() { - return new TableParamsBuilder() - .withKnownMemtables() - .withTransactionalMode() - .withFastPathStrategy() - .build(); + return Generators.toGen(new TableParamsBuilder() + .withKnownMemtables() + .withTransactionalMode() + .withFastPathStrategy() + .build()); } } \ No newline at end of file diff --git a/test/unit/org/apache/cassandra/utils/CassandraGenerators.java b/test/unit/org/apache/cassandra/utils/CassandraGenerators.java index 40903c5273c6..95e601ddfc24 100644 --- a/test/unit/org/apache/cassandra/utils/CassandraGenerators.java +++ b/test/unit/org/apache/cassandra/utils/CassandraGenerators.java @@ -482,7 +482,12 @@ public TableParamsBuilder withFastPathStrategy() int size = SourceDSL.integers().between(1, Integer.MAX_VALUE).generate(rnd); map.put(ParameterizedFastPathStrategy.SIZE, Integer.toString(size)); Set names = new HashSet<>(); - Gen nameGen = SourceDSL.strings().allPossible().ofLengthBetween(1, 10).assuming(s -> !s.trim().isEmpty()); + Gen nameGen = SourceDSL.strings().allPossible().ofLengthBetween(1, 10) + // If : is in the name then the parser will fail; we have validation to disalow this + .map(s -> s.replace(":", "_")) + // Names are used for DCs and those are seperated by , + .map(s -> s.replace(",", "_")) + .assuming(s -> !s.trim().isEmpty()); int numNames = SourceDSL.integers().between(1, 10).generate(rnd); for (int i = 0; i < numNames; i++) { From 6ca42f7ed901885348a655320eb6c880b7c83550 Mon Sep 17 00:00:00 2001 From: David Capwell Date: Mon, 3 Feb 2025 15:36:22 -0800 Subject: [PATCH 2/2] switched to accord testing --- .../tcm/ClusterMetadataSerializerTest.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/unit/org/apache/cassandra/tcm/ClusterMetadataSerializerTest.java b/test/unit/org/apache/cassandra/tcm/ClusterMetadataSerializerTest.java index 1117b4ab8858..97553330de33 100644 --- a/test/unit/org/apache/cassandra/tcm/ClusterMetadataSerializerTest.java +++ b/test/unit/org/apache/cassandra/tcm/ClusterMetadataSerializerTest.java @@ -20,6 +20,7 @@ import org.junit.Test; +import accord.utils.Gen; import org.apache.cassandra.config.DatabaseDescriptor; import org.apache.cassandra.io.util.DataInputBuffer; import org.apache.cassandra.io.util.DataOutputBuffer; @@ -30,11 +31,10 @@ import org.apache.cassandra.tcm.serialization.AsymmetricMetadataSerializers; import org.apache.cassandra.tcm.serialization.Version; import org.apache.cassandra.utils.CassandraGenerators.ClusterMetadataBuilder; +import org.apache.cassandra.utils.Generators; import org.assertj.core.api.Assertions; -import org.quicktheories.core.Gen; -import static org.apache.cassandra.utils.FailingConsumer.orFail; -import static org.quicktheories.QuickTheory.qt; +import static accord.utils.Property.qt; public class ClusterMetadataSerializerTest { @@ -47,16 +47,16 @@ public class ClusterMetadataSerializerTest public void serdeLatest() { DataOutputBuffer output = new DataOutputBuffer(); - qt().forAll(new ClusterMetadataBuilder().build()).checkAssert(orFail(cm -> { + qt().forAll(Generators.toGen(new ClusterMetadataBuilder().build())).check(cm -> { AsymmetricMetadataSerializers.testSerde(output, ClusterMetadata.serializer, cm, NodeVersion.CURRENT_METADATA_VERSION); - })); + }); } @Test public void serdeWithoutAccord() { DataOutputBuffer output = new DataOutputBuffer(); - Gen gen = new ClusterMetadataBuilder().build().assuming(cm -> { + Gen gen = Generators.toGen(new ClusterMetadataBuilder().build()).filter(cm -> { if (!cm.consensusMigrationState.equals(ConsensusMigrationState.EMPTY)) return true; if (!cm.accordStaleReplicas.equals(AccordStaleReplicas.EMPTY)) @@ -65,7 +65,7 @@ public void serdeWithoutAccord() return true; return false; }); - qt().forAll(gen).checkAssert(orFail(cm -> { + qt().forAll(gen).check(cm -> { output.clear(); Version version = Version.V2; // this is the version before accord long expectedSize = ClusterMetadata.serializer.serializedSize(cm, version); @@ -78,6 +78,6 @@ public void serdeWithoutAccord() Assertions.assertThat(read.consensusMigrationState).isEqualTo(ConsensusMigrationState.EMPTY); Assertions.assertThat(read.accordStaleReplicas).isEqualTo(AccordStaleReplicas.EMPTY); Assertions.assertThat(read.accordFastPath).isEqualTo(AccordFastPath.EMPTY); - })); + }); } }