Skip to content

Commit

Permalink
kvclient: switch error in ctx cancel edge case
Browse files Browse the repository at this point in the history
This patch changes the error returned by the DistSender on a cancelled
ctx. Depending on the exactly who detects the ctx as cancelled, there
are a number of different possibilities - too many to enumerate here.
Generally, the client is supposed to get a context.Canceled error,
wrapped in different layers. The code path that this patch changes is
about the cancellation being detected by sendPartialBatch() without it
having been previously detected by sendToReplicas(). This path is
unusual (sendToReplicas() generally detects the ctx cancelled). It's
also hard to test.
Depending on the exact cancellation timing, it's possible though for
sendPartialBatch() to detect it instead. In this case, this patch makes
it so that, if sendToReplicas() returned a sendError (indicating that
a suitable replica could not be reached and that the higher layer is
expected to continue trying other replicas), the error returned to the
client is a cancellation error instead of the sendError.

Touches #69419

Release note: None
  • Loading branch information
andreimatei committed Jan 29, 2022
1 parent bb85ddc commit 2ee8a77
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 5 deletions.
8 changes: 4 additions & 4 deletions pkg/kv/kvclient/kvcoord/dist_sender.go
Original file line number Diff line number Diff line change
Expand Up @@ -1590,9 +1590,6 @@ func (ds *DistSender) sendPartialBatch(
}

if err != nil {
// Set pErr so that, if we don't perform any more retries, the
// deduceRetryEarlyExitError() call below the loop is inhibited.
pErr = roachpb.NewError(err)
switch {
case errors.HasType(err, sendError{}):
// We've tried all the replicas without success. Either they're all
Expand All @@ -1614,6 +1611,9 @@ func (ds *DistSender) sendPartialBatch(
routingTok.Evict(ctx)
continue
}
// Set pErr so that the deduceRetryEarlyExitError() call below the loop is
// inhibited.
pErr = roachpb.NewError(err)
break
}

Expand Down Expand Up @@ -1677,7 +1677,7 @@ func (ds *DistSender) sendPartialBatch(
// channels were closed.
if pErr == nil {
if err := ds.deduceRetryEarlyExitError(ctx); err == nil {
log.Fatal(ctx, "exited retry loop without an error")
pErr = roachpb.NewError(errors.AssertionFailedf("exited retry loop without an error"))
} else {
pErr = roachpb.NewError(err)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/kv/kvserver/client_merge_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4127,7 +4127,7 @@ func TestStoreRangeMergeDuringShutdown(t *testing.T) {
// Send a dummy get request on the RHS to force a lease acquisition. We expect
// this to fail, as quiescing stores cannot acquire leases.
_, err = store.DB().Get(ctx, key.Next())
if exp := "not lease holder"; !testutils.IsError(err, exp) {
if exp := "node unavailable"; !testutils.IsError(err, exp) {
t.Fatalf("expected %q error, but got %v", exp, err)
}
}
Expand Down

0 comments on commit 2ee8a77

Please sign in to comment.