kv: deflake TestReplicaStateMachineChangeReplicas #69730
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes cockroachdb#69229. The test was reaching below Raft and adding a peer on a node that does not exist. With a large enough pause between `r.raftMu.Unlock()` and `testContext` teardown, it was possible for the replica to be ticked, which would cause it to attempt to reach out and heartbeat this node. This could cause issues. In fact, the `testContext` uses a dummy `RaftTransport`, so any attempt to send traffic to another Raft peer would panic. This commit resolves the issue by giving testContext a slightly more real version of a RaftTransport (i.e. one configured with a nodedialer). This feels a bit wrong, as a testContext should really never need to use a RaftTransport, but given how little impact this has on the dependency structure of a testContext and given that the testContext was already doing something similar with gossip, I think this is fine. Release justification: test fix.
|
This change is |
tbg
approved these changes
Sep 2, 2021
|
bors r+ |
|
Build succeeded: |
tbg
added a commit
to tbg/cockroach
that referenced
this pull request
Dec 8, 2022
Some tests do add peers and so we'll see some use of the raft transport. This doesn't have to work properly, just not crash, which this achieves. This is essentially a re-do of cockroachdb#69730 which was lost in cockroachdb#72383. Release note: None
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #69229.
The test was reaching below Raft and adding a peer on a node that does not
exist. With a large enough pause between
r.raftMu.Unlock()andtestContextteardown, it was possible for the replica to be ticked, which would cause it to
attempt to reach out and heartbeat this node. This could cause issues. In fact,
the
testContextuses a dummyRaftTransport, so any attempt to send trafficto another Raft peer would panic.
This commit resolves the issue by giving testContext a slightly more real
version of a RaftTransport (i.e. one configured with a nodedialer). This feels a
bit wrong, as a testContext should really never need to use a RaftTransport, but
given how little impact this has on the dependency structure of a testContext
and given that the testContext was already doing something similar with gossip,
I think this is fine.
Release justification: test fix.