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.

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

release-22.2: logictestccl: Fix regional_by_row_query_behavior flake #105787

Merged

Conversation

msirek
Copy link
Contributor

@msirek msirek commented Jun 28, 2023

Backport:

Please see individual PRs for details.

/cc @cockroachdb/release


logictest: only purge zone configs for in-memory test clusters

This commit backports code needed to fix #104466.

Informs: #104466

Release note: None


logictest: retry support for "statement" and "query" commands
This adds a retry command to logic tests which may be issued on a
separate line preceding either a "statement" or "query" command.
It causes the statement to be retried with exponential backoff up
to some maximum duration, e.g.

retry
statement error column "non_exist" does not exist
ALTER TABLE created_as_global SET LOCALITY REGIONAL BY ROW AS "non_exist"

This has the same effect as the retry option of the query command, but
now also supports "statement ok", "statement error" and "query error"
commands.

Retry of a query command may be specified by the standalone retry
command, the retry option of the query command, or both.

Fixes: #95668

Epic: CRDB-20535

Release note: None


logictest: retry support for "statement" and "query" commands

This adds a retry command to logic tests which may be issued on a
separate line preceding either a "statement" or "query" command.
It causes the statement to be retried with exponential backoff up
to some maximum duration, e.g.

retry
statement error column "non_exist" does not exist
ALTER TABLE created_as_global SET LOCALITY REGIONAL BY ROW AS "non_exist"

This has the same effect as the retry option of the query command, but
now also supports "statement ok", "statement error" and "query error"
commands.

Retry of a query command may be specified by the standalone retry
command, the retry option of the query command, or both.

Fixes: #95668

Epic: CRDB-20535

Release note: None


logictestccl: add more retries to logic tests

This commit adds more retries to multiregion logic tests.

Fixes #104466

Release note: None


Release justification: Fix #104466

@blathers-crl
Copy link

blathers-crl bot commented Jun 28, 2023

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Mark Sirek and others added 4 commits June 28, 2023 17:00
Fixes cockroachdb#86284

This fixes a test flake in regional_by_row_query_behavior which is
caused by 2 issues:

  1. Zone configs not becoming available in `SystemConfig` between
     table creation time and query execution time.

  2. Metamorphic testing modify the `coldata-batch-size` setting,
     causing a different number of kv reads to be done, which affects
     the kv trace results in this test.

The flake can be reproduced via the command:
```
./dev testlogic ccl --files=regional_by_row_query_behavior \
                    --config=multiregion-9node-3region-3azs --stress
```
For point 1, the `retry` logic test directive is enhanced to purge the
zone config cache in the SystemConfig when a failing test is retried,
allowing the updated zone config to be read.

For point 2, the logic test blocking flag `!metamorphic` which resets
batch size related settings that were modified in metamorphic mode back
to their default production values, is updated to also cover the
`coldata-batch-size` setting. The flag is renamed to
`!metamorphic-batch-sizes` to better reflect its actual effect.

Release justification: low risk fix for test flake

Release note: None
Fixes cockroachdb#88398

This fixes a panic which can occur duing cluster startup in a logic test
or during retry of a test case in a logic test when an attempt is made
to purge the zone config cache and no SystemConfig is available.

Release note: None
Previously, by mistake we were acquiring a read lock even though we do
write operations in `PurgeZoneConfigCache`.

Release note: None
This commit backports code needed to fix cockroachdb#104466.

Informs: cockroachdb#104466

Release note: None
@msirek msirek force-pushed the backport22.2-87391-90802-92606 branch from 93ccffa to e5e341e Compare June 29, 2023 00:53
Mark Sirek added 2 commits June 28, 2023 22:56
This adds a retry command to logic tests which may be issued on a
separate line preceding either a "statement" or "query" command.
It causes the statement to be retried with exponential backoff up
to some maximum duration, e.g.
```
retry
statement error column "non_exist" does not exist
ALTER TABLE created_as_global SET LOCALITY REGIONAL BY ROW AS "non_exist"
```
This has the same effect as the retry option of the query command, but
now also supports "statement ok", "statement error" and "query error"
commands.

Retry of a query command may be specified by the standalone retry
command, the retry option of the query command, or both.

Fixes: cockroachdb#95668

Epic: CRDB-20535

Release note: None
This commit adds more retries to multiregion logic tests.

Fixes cockroachdb#104466

Release note: None
@msirek msirek force-pushed the backport22.2-87391-90802-92606 branch from e5e341e to 1785349 Compare June 29, 2023 05:56
@msirek msirek requested a review from yuzefovich June 29, 2023 14:25
@msirek msirek marked this pull request as ready for review June 29, 2023 14:25
@msirek msirek requested review from a team as code owners June 29, 2023 14:25
Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

Reviewed 16 of 16 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @msirek)

@msirek
Copy link
Contributor Author

msirek commented Jun 30, 2023

TFTR!

@msirek msirek merged commit 4b5144b into cockroachdb:release-22.2 Jun 30, 2023
@msirek msirek deleted the backport22.2-87391-90802-92606 branch June 30, 2023 00:06
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.

3 participants