Skip to content

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
dcapwell wants to merge 2 commits intoapache:cep-15-accordfrom
dcapwell:CASSANDRA-20286

Conversation

@dcapwell
Copy link
Copy Markdown
Contributor

@dcapwell dcapwell commented Feb 3, 2025

No description provided.

…ors and production validation logic missed the argument to String.format causing confusing errors
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
Copy Markdown
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

{
DataOutputBuffer output = new DataOutputBuffer();
qt().forAll(tableParams()).checkAssert(FailingConsumer.orFail(params -> {
qt().forAll(tableParams()).check(params -> {
Copy link
Copy Markdown
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....

{
DataOutputBuffer output = new DataOutputBuffer();
qt().forAll(new ClusterMetadataBuilder().build()).checkAssert(orFail(cm -> {
qt().forAll(Generators.toGen(new ClusterMetadataBuilder().build())).check(cm -> {
Copy link
Copy Markdown
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....

{
DataOutputBuffer output = new DataOutputBuffer();
Gen<ClusterMetadata> gen = new ClusterMetadataBuilder().build().assuming(cm -> {
Gen<ClusterMetadata> gen = Generators.toGen(new ClusterMetadataBuilder().build()).filter(cm -> {
Copy link
Copy Markdown
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

Comment on lines +487 to +489
.map(s -> s.replace(":", "_"))
// Names are used for DCs and those are seperated by ,
.map(s -> s.replace(",", "_"))
Copy link
Copy Markdown
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

@dcapwell dcapwell requested a review from bdeggleston February 3, 2025 23:50
Copy link
Copy Markdown
Member

@bdeggleston bdeggleston left a comment

Choose a reason for hiding this comment

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

+1

@dcapwell
Copy link
Copy Markdown
Contributor Author

dcapwell commented Feb 6, 2025

merged 3089226

@dcapwell dcapwell closed this Feb 6, 2025
@dcapwell dcapwell deleted the CASSANDRA-20286 branch February 6, 2025 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants