kvserver: improve the delegate snapshot code#82490
Merged
craig[bot] merged 5 commits intocockroachdb:masterfrom Jun 8, 2022
Merged
kvserver: improve the delegate snapshot code#82490craig[bot] merged 5 commits intocockroachdb:masterfrom
craig[bot] merged 5 commits intocockroachdb:masterfrom
Conversation
Member
|
This change is |
aayushshah15
reviewed
Jun 8, 2022
Contributor
aayushshah15
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @nvanbenschoten)
pkg/kv/kvserver/raft_transport.go line 468 at r4 (raw file):
// RaftSnapshot handles incoming streaming snapshot requests. func (t *RaftTransport) RaftSnapshot(stream MultiRaft_RaftSnapshotServer) error {
We should update this handler as well right?
A span created for the handling of the DelegateRaftSnapshot RPC was sometimes leaked when the RPC was racing with the server shutdown. This is because the resposibility of closing this span was delegated over several layers, and two of the layers were not closing it when they were failing to create a stopper task. The span in question was also created with the wrong name because of copy-pasta. Also, the span is generally unnecessary; I'll clean up in a further patch. Fixes cockroachdb#80838 Fixes cockroachdb#80822 Release note: None
99442d6 to
6115998
Compare
andreimatei
commented
Jun 8, 2022
Contributor
Author
andreimatei
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/raft_transport.go line 468 at r4 (raw file):
Previously, aayushshah15 (Aayush Shah) wrote…
We should update this handler as well right?
done
aayushshah15
approved these changes
Jun 8, 2022
Contributor
aayushshah15
left a comment
There was a problem hiding this comment.
Reviewable status:
The DelegateRaftSnapshot RPC handler was creating a tracing span even though a similar one had already been created by the gRPC tracing interceptor. Release note: None
Store.HandleDelegatedSnapshot was operating in a Task even though its one caller is also creating a Task. Release note: None
The handler for these RPC was spawning off work asynchronously, and waiting on it to be done, while also waiting on stopped shutdown. At the same time, it was also configuring the ctx to be canceled on stopper quiescence. As far as I can tell, the work in question responds to cancelation, so having the sender separately wait for server shutdown was not necessary. Even more, I don't think it was valid to return from the handler while the RPC's stream is in use by an another goroutine (although gRPC does not document these contracts well). This patch makes the RPC handler run synchronously. No (synchronous) stopper task is necessary since all the RPCs get one through a handler. Release note: None
Release note: None
6115998 to
72986a8
Compare
andreimatei
commented
Jun 8, 2022
Contributor
Author
andreimatei
left a comment
There was a problem hiding this comment.
bors r+
Reviewable status:
Contributor
|
Build succeeded: |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
See individual commits.