Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CASSANDRA-20286: Accord: TableParamsTest is flakey due to bad generators and production validation logic missed the argument to String.format causing confusing errors #3865

Closed
wants to merge 2 commits into from
Closed

CASSANDRA-20286: Accord: TableParamsTest is flakey due to bad generators and production validation logic missed the argument to String.format causing confusing errors #3865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d224262
CASSANDRA-20286: Accord: TableParamsTest is flakey due to bad generat…
dcapwell Feb 3, 2025
6ca42f7
switched to accord testing
dcapwell Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/java/org/apache/cassandra/service/accord/fastpath/ParameterizedFastPathStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <dc>:<weight>");
throw cfe("Invalid dc weighting syntax %s, use <dc>:<weight>", s);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was the error we saw in UI, and its because foo:bar:42... the dc had : in the name, so hit this logic, but since , s was missing it threw an issue with string formatting rather than the CFE we expected

}
}

Expand Down
20 changes: 10 additions & 10 deletions test/unit/org/apache/cassandra/schema/TableParamsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 -> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

accord qt will show the seed that failed when generators fail, but QuickTheories doesn't; making it hard to reproduce these type of errors.

I plan to migrate all tests off of QuickTheories after we merge to trunk, and these are cep-15-accord only tests....

AsymmetricMetadataSerializers.testSerde(output, TableParams.serializer, params, NodeVersion.CURRENT_METADATA_VERSION);
}));
});
}

private static Gen<TableParams> tableParams()
{
return new TableParamsBuilder()
.withKnownMemtables()
.withTransactionalMode()
.withFastPathStrategy()
.build();
return Generators.toGen(new TableParamsBuilder()
.withKnownMemtables()
.withTransactionalMode()
.withFastPathStrategy()
.build());
}
}
16 changes: 8 additions & 8 deletions test/unit/org/apache/cassandra/tcm/ClusterMetadataSerializerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
{
Expand All @@ -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 -> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

accord qt will show the seed that failed when generators fail, but QuickTheories doesn't; making it hard to reproduce these type of errors.

I plan to migrate all tests off of QuickTheories after we merge to trunk, and these are cep-15-accord only tests....

AsymmetricMetadataSerializers.testSerde(output, ClusterMetadata.serializer, cm, NodeVersion.CURRENT_METADATA_VERSION);
}));
});
}

@Test
public void serdeWithoutAccord()
{
DataOutputBuffer output = new DataOutputBuffer();
Gen<ClusterMetadata> gen = new ClusterMetadataBuilder().build().assuming(cm -> {
Gen<ClusterMetadata> gen = Generators.toGen(new ClusterMetadataBuilder().build()).filter(cm -> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

accord filter doesn't have a bounded amount of retries but accuming with QuickTheories does, making this test flakey. The issue is when a CM is generated without anything accord related... we filter those out... if you get unlucky and do this many times in a row assuming would reject the build; filter does not

if (!cm.consensusMigrationState.equals(ConsensusMigrationState.EMPTY))
return true;
if (!cm.accordStaleReplicas.equals(AccordStaleReplicas.EMPTY))
Expand All @@ -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);
Expand All @@ -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);
}));
});
}
}
7 changes: 6 additions & 1 deletion test/unit/org/apache/cassandra/utils/CassandraGenerators.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> names = new HashSet<>();
Gen<String> nameGen = SourceDSL.strings().allPossible().ofLengthBetween(1, 10).assuming(s -> !s.trim().isEmpty());
Gen<String> 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(",", "_"))
Comment on lines +487 to +489
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are the real bug fixes.

: caused dc:weight parser to fail (we expect 2, but had 3)
, caused issues as we had a dc a,b

These are generator bugs and not allowed from CQL layer... so switched to _ to avoid shrinking the size

.assuming(s -> !s.trim().isEmpty());
int numNames = SourceDSL.integers().between(1, 10).generate(rnd);
for (int i = 0; i < numNames; i++)
{
Expand Down