From 397409c9fb8bc3724980443849746256c8ef72af Mon Sep 17 00:00:00 2001 From: Alex Robinson Date: Wed, 12 Sep 2018 23:07:43 +0000 Subject: [PATCH] kv: Don't evict from leaseholder cache due to context cancellations This was a major contributor to the hundreds of NotLeaseHolderErrors per second that we see whenever we run tpc-c at scale. A non-essential batch request like a QueryTxn would get cancelled, causing the range to be evicted from the leaseholder cache and the next request to that range to have to guess at the leaseholder. Release note: None --- pkg/kv/dist_sender.go | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/pkg/kv/dist_sender.go b/pkg/kv/dist_sender.go index 84036983ba46..6d356a836137 100644 --- a/pkg/kv/dist_sender.go +++ b/pkg/kv/dist_sender.go @@ -1324,20 +1324,21 @@ func (ds *DistSender) sendToReplicas( ambiguousError = err } log.VErrEventf(ctx, 2, "RPC error: %s", err) - if storeID, ok := ds.leaseHolderCache.Lookup(ctx, rangeID); ok && curReplica.StoreID == storeID { - // If the down replica is cached as the lease holder, evict - // it. The only other eviction happens below on - // NotLeaseHolderError, but if the next replica is the - // actual lease holder, we're never going to receive one of - // those and will thus pay the price of trying the down node - // first forever. - // - // NB: we could consider instead adding a successful reply - // from the next replica into the cache, but without a - // leaseholder (and taking into account that the local - // node can't be down) it won't take long until we talk - // to a replica that tells us who the leaseholder is. - ds.leaseHolderCache.Update(ctx, rangeID, 0 /* evict */) + + // If the error wasn't just a context cancellation and the down replica + // is cached as the lease holder, evict it. The only other eviction + // happens below on NotLeaseHolderError, but if the next replica is the + // actual lease holder, we're never going to receive one of those and + // will thus pay the price of trying the down node first forever. + // + // NB: we should consider instead adding a successful reply from the next + // replica into the cache, but without a leaseholder (and taking into + // account that the local node can't be down) it won't take long until we + // talk to a replica that tells us who the leaseholder is. + if ctx.Err() == nil { + if storeID, ok := ds.leaseHolderCache.Lookup(ctx, rangeID); ok && curReplica.StoreID == storeID { + ds.leaseHolderCache.Update(ctx, rangeID, 0 /* evict */) + } } } else { propagateError := false