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

Revert "kvprober: metamorphically enable / configure kvprober" #108454

Merged

Conversation

nvanbenschoten
Copy link
Member

This reverts (most of) commit 769ba1c.

That commit metamorphically enabled kvprober. This has been observed to be destabliziing to unit tests. When the metamorphic constant is enabled (50% of the time) and when kvprober is fast enough, random ranges will see extra requests that they aren’t expecting. This adds nondeterminism which can trip up tests in any number of different ways.

All of the following flakes have been tracked back to kvprober:

Fixes #107864.
Fixes #108242.
Fixes #108441.
Fixes #108349.
Fixes #108124.
Closes #108366.

Release note: None

This reverts (most of) commit 769ba1c.

That commit metamorphically enabled kvprober. This has been observed to
be destabliziing to unit tests. When the metamorphic constant is
enabled (50% of the time) and when kvprober is fast enough, random
ranges will see extra requests that they aren’t expecting. This adds
nondeterminism which can trip up tests in any number of different ways.

All of the following flakes have been tracked back to kvprober:

Fixes cockroachdb#107864.
Fixes cockroachdb#108242.
Fixes cockroachdb#108441.
Fixes cockroachdb#108349.
Fixes cockroachdb#108124.
Closes cockroachdb#108366.

Release note: None
@blathers-crl
Copy link

blathers-crl bot commented Aug 9, 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
Collaborator

@andrewbaptist andrewbaptist left a comment

Choose a reason for hiding this comment

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

:lgtm:

I agree that this adds too much non-determinism into tests, especially at the KV layer and below. We could consider a change like this for SQL tests, but I'm not sure how useful that would be in practice.

Copy link
Collaborator

@joshimhoff joshimhoff left a comment

Choose a reason for hiding this comment

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

Sorry for the trouble & thx for tracking this down & fixing.

To be clear about it, enabling kvprober leads to test failures due to the fact that the tests do very specific things and assume that there is no / minimal background querying of ranges, etc. But we do not see an bad behavior caused by kvprober. Right or wrong?

@nvanbenschoten
Copy link
Member Author

TFTRs!

bors r=andrewbaptist,joshimhoff

To be clear about it, enabling kvprober leads to test failures due to the fact that the tests do very specific things and assume that there is no / minimal background querying of ranges, etc. But we do not see an bad behavior caused by kvprober. Right or wrong?

Yes, that's correct. We saw no indication that kvprober itself had issues.

@craig
Copy link
Contributor

craig bot commented Aug 9, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 9, 2023

Build succeeded:

@craig craig bot merged commit b91c919 into cockroachdb:master Aug 9, 2023
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