kvserver: don't use caller's context for ProposalData#78174
kvserver: don't use caller's context for ProposalData#78174craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
d60e631 to
0610f35
Compare
erikgrinaker
left a comment
There was a problem hiding this comment.
LGTM, thanks! Confirmed that tracing still works as expected.
It's a bit odd that the assertion is buried deep within some tracing method, but this is where we assign the command context and it makes sense to keep the patch small for backporting. We should consider restructuring this a bit later.
Previously, for local proposals, we were threading the caller's context down into Raft. This has caused a few bugs related to context cancellation sensitivity - the execution of a raft command should not be impacted by whether or not the client's context has already finished. This commit derives the proposal context from the Replica's annotated context, using a tracing span derived from the client context. That way, the derived context is not cancelable, but contains the same tracing Span (thus providing similar utility) as before. Notably, the trace span should also already contain the log tags present in the client's context. The tracing has coverage through (at least) a few tests that make assertions about trace events from below-raft showing up in a trace collected by the client, which I verified by omitting the Span: ``` TestRaftSSTableSideloadingProposal TestReplicaAbandonProposal TestReplicaRetryRaftProposal TestReplicaBurstPendingCommandsAndRepropose TestReplicaReproposalWithNewLeaseIndexError TestFailureToProcessCommandClearsLocalResult TestDBAddSSTable ``` See cockroachdb#75656. Release note: None
0610f35 to
825a4ee
Compare
|
Had to rework this a bit (CI failed) - by creating the span above raft, we were starting to leak spans, and so there would have to have been more investigation and code. In the interest of time and keeping it small, I moved span creation below raft again. This is arguably the better change anyway, though now there is PTAQL |
erikgrinaker
left a comment
There was a problem hiding this comment.
Yeah, this is fine for now.
|
Failure is TestLogic/fakedist/enums bors r=[erikgrinaker,nvanbenschoten] |
|
Build failed (retrying...): |
|
Build succeeded: |
Fixes #75656.
Release note: None