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

batcheval: modify MVCCStats CleanRange assertion under race #95694

Merged
merged 1 commit into from
Jan 23, 2023

Conversation

adityamaru
Copy link
Contributor

Previously, if ClearRange took a fast path and was being tested under race, then we would assert that the computed MVCCStats were the same as those of the fast path. This would not be true if the MVCCStats contained estimates which they do in operation such as cluster to cluster replication.

This change tweaks the assertion to only run if the MVCCStats do not contain estimates since if they do there is no guarantee they will match up with the computed MVCCStats.

Fixes: #95656
Fixes: #95677

Release note: None

@adityamaru adityamaru requested a review from a team as a code owner January 23, 2023 18:59
@blathers-crl
Copy link

blathers-crl bot commented Jan 23, 2023

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Previously, if ClearRange took a fast path and was being tested
under race, then we would assert that the computed MVCCStats were
the same as those of the fast path. This would not be true if the
MVCCStats contained estimates which they do in operation such as
cluster to cluster replication.

This change tweaks the assertion to only run if the MVCCStats do
not contain estimates since if they do there is no guarantee they will
match up with the computed MVCCStats.

Fixes: cockroachdb#95656
Fixes: cockroachdb#95677

Release note: None
@adityamaru
Copy link
Contributor Author

We still need the line computed.ContainsEstimates = delta.ContainsEstimates // retained for tests under race because the ContainsEstimates value is used to determine whether or not we use the fast path. So in
TestCmdClearRange/estimated_stats/spanningRangeTombstones=true because we don't copy the value of ContainsEstimates to the computed struct we would not see the expected number of invocation of ClearMVCCIteratorRange

@adityamaru
Copy link
Contributor Author

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Jan 23, 2023

Build succeeded:

@craig craig bot merged commit efd23f5 into cockroachdb:master Jan 23, 2023
@adityamaru adityamaru deleted the fix-assertion branch January 24, 2023 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants