From 50b80e3684b5d4ba518f95ff875b546682da0cb5 Mon Sep 17 00:00:00 2001 From: Aayush Shah Date: Wed, 8 Sep 2021 13:42:28 -0400 Subject: [PATCH] kvserver: improve a comment in `TransferLeaseTarget` Release justification: adds no-op safeguard Release note: None --- pkg/kv/kvserver/allocator.go | 7 ++++++- pkg/kv/kvserver/replicate_queue.go | 11 ++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/pkg/kv/kvserver/allocator.go b/pkg/kv/kvserver/allocator.go index 694347335798..f076a540007a 100644 --- a/pkg/kv/kvserver/allocator.go +++ b/pkg/kv/kvserver/allocator.go @@ -1451,7 +1451,7 @@ func (a *Allocator) TransferLeaseTarget( leaseholderReplQPS, _ := stats.avgQPS() currentDelta := getQPSDelta(storeQPSMap, existing) bestOption := getCandidateWithMinQPS(storeQPSMap, existing) - if bestOption != (roachpb.ReplicaDescriptor{}) && + if bestOption != (roachpb.ReplicaDescriptor{}) && leaseholderReplQPS > 0 && // It is always beneficial to transfer the lease to the coldest candidate // if the range's own qps is smaller than the difference between the // leaseholder store and the candidate store. This will always drive down @@ -1466,6 +1466,11 @@ func (a *Allocator) TransferLeaseTarget( // ranges with low QPS. This can add up and prevent us from achieving // convergence in cases where we're dealing with a ton of very low-QPS // ranges. + // + // NB: If the `bestOption` ends up being the current leaseholder, then the + // check below will fail since we know that `leaseholderReplQPS` must be + // greater than 0 (the `StoreRebalancer` only bothers rebalancing leases / + // replicas for ranges serving at least some traffic). (leaseholderStoreQPS-leaseholderReplQPS) > storeQPSMap[bestOption.StoreID] { storeQPSMap[leaseRepl.StoreID()] -= leaseholderReplQPS storeQPSMap[bestOption.StoreID] += leaseholderReplQPS diff --git a/pkg/kv/kvserver/replicate_queue.go b/pkg/kv/kvserver/replicate_queue.go index 734f514b0c8d..75e12ffcaa48 100644 --- a/pkg/kv/kvserver/replicate_queue.go +++ b/pkg/kv/kvserver/replicate_queue.go @@ -1284,10 +1284,15 @@ const ( ) type transferLeaseOptions struct { - goal transferLeaseGoal + goal transferLeaseGoal + // checkTransferLeaseSource, when false, tells `TransferLeaseTarget` to + // exclude the current leaseholder from consideration as a potential target + // (i.e. when the caller explicitly wants to shed its lease away). checkTransferLeaseSource bool - checkCandidateFullness bool - dryRun bool + // checkCandidateFullness, when false, tells `TransferLeaseTarget` + // to disregard the existing lease counts on candidates. + checkCandidateFullness bool + dryRun bool } // leaseTransferOutcome represents the result of shedLease().