Skip to content

Commit

Permalink
kv: don't leak raft application tracing spans on ErrRemoved
Browse files Browse the repository at this point in the history
Fixes cockroachdb#53677.

This commit ensures that we properly finish tracing spans of
Raft commands that throw ErrRemoved errors in ApplySideEffects.

I was originally intending to do something more dramatic and make
`replicaStateMachine.ApplySideEffects` responsible for acknowledging
proposers in all cases, but doing so turned out to be pretty invasive
so I was concerned that it would be harder to backport to v20.2 and
to v20.1. I may revisit that in the future.

Release justification: low risk, high benefit changes to existing
functionality.
  • Loading branch information
nvanbenschoten committed Sep 11, 2020
1 parent b175745 commit 1a0ad39
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 2 deletions.
16 changes: 15 additions & 1 deletion pkg/kv/kvserver/replica_application_cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,13 +155,27 @@ func (c *replicatedCmd) AckSuccess() error {

// FinishAndAckOutcome implements the apply.AppliedCommand interface.
func (c *replicatedCmd) FinishAndAckOutcome(ctx context.Context) error {
tracing.FinishSpan(c.sp)
if c.IsLocal() {
c.proposal.finishApplication(ctx, c.response)
}
c.finishTracingSpan()
return nil
}

// FinishNonLocal is like AckOutcomeAndFinish, but instead of acknowledging the
// command's proposal if it is local, it asserts that the proposal is not local.
func (c *replicatedCmd) FinishNonLocal(ctx context.Context) {
if c.IsLocal() {
log.Fatalf(ctx, "proposal unexpectedly local: %v", c.replicatedResult())
}
c.finishTracingSpan()
}

func (c *replicatedCmd) finishTracingSpan() {
tracing.FinishSpan(c.sp)
c.ctx, c.sp = nil, nil
}

// decode decodes the entry e into the decodedRaftEntry.
func (d *decodedRaftEntry) decode(ctx context.Context, e *raftpb.Entry) error {
*d = decodedRaftEntry{}
Expand Down
4 changes: 3 additions & 1 deletion pkg/kv/kvserver/replica_application_state_machine.go
Original file line number Diff line number Diff line change
Expand Up @@ -1044,8 +1044,10 @@ func (sm *replicaStateMachine) ApplySideEffects(
clearTrivialReplicatedEvalResultFields(cmd.replicatedResult())
if !cmd.IsTrivial() {
shouldAssert, isRemoved := sm.handleNonTrivialReplicatedEvalResult(ctx, cmd.replicatedResult())

if isRemoved {
// The proposal must not have been local, because we don't allow a
// proposing replica to remove itself from the Range.
cmd.FinishNonLocal(ctx)
return nil, apply.ErrRemoved
}
// NB: Perform state assertion before acknowledging the client.
Expand Down

0 comments on commit 1a0ad39

Please sign in to comment.