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

kvserver: remove TestReplicaRemovalClosesProposalQuota #98685

Merged
merged 2 commits into from
Mar 16, 2023

Conversation

tbg
Copy link
Member

@tbg tbg commented Mar 15, 2023

As this test explained in its comment, the circumstances it was
seeking to test were very rarely met and even if the behavior
were missing, this wouldn't cause any problems.

The test has been skipped (accidentally) for a long time, was flaky even
then (leaking trace spans, see below), and it's not easy to
enact the situation the test wishes to construct in an idiomatic way.

It's pretty clear that the lines the test wants are present:

if pq := r.mu.proposalQuota; pq != nil {
pq.Close("destroyed")
}

It's better to remove the test at this point.

Closes #96932.

Also, fix a buglet that the test "accidentally" uncovered:

Previously, if evalAndPropose returned with an error while trying to
propose a pipelined write, it would leak the trace span.

Make sure this doesn't happen. The test that exercised this got deleted
in the previous commit, but this is still a bug that could cause a
larger leak if a condition were ever added to evalAndPropose which
could cause a large amount of errors (perhaps spread out over a longer
time period) in production clusters.

Unfortunately, writing a test for this is likely to be a net negative;
if the reviewer feels strongly I can add a testing knob to inject an
error in the right place in this method and exercise it that way.

Touches #96932.

Epic: none
Release note: None

As this test explained in its comment, the circumstances it was
seeking to test were very rarely met and even if the behavior
were missing, this wouldn't cause any problems.

The test has been skipped (accidentally) for a long time, was flaky even
then (leaking trace spans, see follow-up commit), and it's not easy to
enact the situation the test wishes to construct in an idiomatic way.

It's pretty clear that the lines the test wants are present:

https://github.com/cockroachdb/cockroach/blob/736a67e0d36cc545bf74d65db069ee895ff9bea0/pkg/kv/kvserver/replica_destroy.go#L174-L176

It's better to remove the test at this point.

Closes cockroachdb#96932.

Epic: none
Release note: None
@tbg tbg requested a review from a team March 15, 2023 16:01
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg tbg requested a review from aliher1911 March 15, 2023 16:01
Previously, if `evalAndPropose` returned with an error while trying to
propose a pipelined write, it would leak the trace span.

Make sure this doesn't happen. The test that exercised this got deleted
in the previous commit, but this is still a bug that could cause a
larger leak if a condition were ever added to `evalAndPropose` which
could cause a large amount of errors (perhaps spread out over a longer
time period) in production clusters.

Unfortunately, writing a test for this is likely to be a net negative;
if the reviewer feels strongly I can add a testing knob to inject an
error in the right place in this method and exercise it that way.

Touches cockroachdb#96932.

Epic: none
Release note: None
@tbg
Copy link
Member Author

tbg commented Mar 16, 2023

bors r=pavelkalinnikov

@craig
Copy link
Contributor

craig bot commented Mar 16, 2023

Build succeeded:

@craig craig bot merged commit b6978e9 into cockroachdb:master Mar 16, 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
Development

Successfully merging this pull request may close these issues.

kv/kvserver: TestReplicaRemovalClosesProposalQuota failed
3 participants