From c725710d2a78b2395e4594eb800478ade494eedc Mon Sep 17 00:00:00 2001 From: Austen McClernon Date: Fri, 9 Feb 2024 10:47:20 -0500 Subject: [PATCH] allocatorimpl: return why lease should be transferred `ShouldTransferLease` is used to determine whether a replica should be enqueued for a later lease transfer. The reason the lease should be transferred could be due to preferences, IO overload or balance. Include the reason why the lease should be transferred, or why the lease should not be transferred. Part of: #118966 Release note: None --- .../allocator/allocatorimpl/allocator.go | 125 +++++++++++-- .../allocator/allocatorimpl/allocator_test.go | 166 +++++++++--------- pkg/kv/kvserver/allocator/plan/replicate.go | 13 +- .../lint/passes/redactcheck/redactcheck.go | 7 +- 4 files changed, 206 insertions(+), 105 deletions(-) diff --git a/pkg/kv/kvserver/allocator/allocatorimpl/allocator.go b/pkg/kv/kvserver/allocator/allocatorimpl/allocator.go index 86b1902c1382..9aa1eb43131a 100644 --- a/pkg/kv/kvserver/allocator/allocatorimpl/allocator.go +++ b/pkg/kv/kvserver/allocator/allocatorimpl/allocator.go @@ -366,10 +366,10 @@ func (s ReplicaStatus) String() string { // SafeValue implements the redact.SafeValue interface. func (t ReplicaStatus) SafeValue() {} -type transferDecision int +type accessLocalityTransferDecision int const ( - _ transferDecision = iota + _ accessLocalityTransferDecision = iota shouldTransfer shouldNotTransfer decideWithoutStats @@ -2523,6 +2523,100 @@ func getLoadDelta( return maxCandidateLoad - minCandidateLoad } +// TransferLeaseDecision indicates whether a range lease should be transferred +// and if so, for what reason. +type TransferLeaseDecision int + +const ( + _ TransferLeaseDecision = iota + // DontTransferLeaseCountBalanced indicates the load/lease counts of the + // valid replica stores are balanced within the target threshold and + // therefore the lease should not be transferred. + DontTransferLeaseBalanced + // DontTransferLeaseNoValidTargets indicates the lease should no be + // transferred from the current leaseholder because there are no valid + // leaseholder targets. + DontTransferLeaseNoValidTargets + // DontTransferLeaseNoStoreDescriptor indicates the lease should not be + // transferred because the current leaseholder's store descriptor cannot be + // found. This can occur on startup, before gossip has propogated the local + // descriptor. + DontTransferLeaseNoStoreDescriptor + // TransferLeaseForCountBalance indicates the lease should be transferred to + // better balance lease counts. + TransferLeaseForCountBalance + // TransferLeaseForAccessLocality indicates the lease should be transferred + // for better access locality. + TransferLeaseForAccessLocality + // TransferLeaseForIOOverload indicates the lease should be transferred + // because the current leaseholder's store is IO overloaded. + TransferLeaseForIOOverload + // TransferLeaseForPreferences indicates the lease should be transferred + // because there is a more preferred leaseholder according the applied range + // lease preferences. + TransferLeaseForPreferences +) + +// ShouldTransfer returns true when the lease should be transferred, false +// otherwise. +func (t TransferLeaseDecision) ShouldTransfer() bool { + switch t { + case TransferLeaseForCountBalance, TransferLeaseForAccessLocality, + TransferLeaseForIOOverload, TransferLeaseForPreferences: + return true + case DontTransferLeaseBalanced, DontTransferLeaseNoValidTargets, + DontTransferLeaseNoStoreDescriptor: + return false + default: + panic(fmt.Sprintf("unknown transfer lease decision %d", t)) + } +} + +// Priority returns the relative urgency of the lease transfer decision. The +// priority may be used to determine the ordering of lease transfers when +// multiple should occur. +func (t TransferLeaseDecision) Priority() float64 { + switch t { + case TransferLeaseForPreferences: + return 300 + case TransferLeaseForIOOverload: + return 200 + case TransferLeaseForAccessLocality: + return 100 + case TransferLeaseForCountBalance: + return 0 + case DontTransferLeaseBalanced, DontTransferLeaseNoValidTargets, + DontTransferLeaseNoStoreDescriptor: + return 0 + default: + panic(fmt.Sprintf("unknown transfer lease decision %d", t)) + } +} + +// SafeValue implements the redact.SafeValue interface. +func (t TransferLeaseDecision) SafeValue() {} + +func (t TransferLeaseDecision) String() string { + switch t { + case TransferLeaseForCountBalance: + return "transfer(lease count)" + case TransferLeaseForAccessLocality: + return "transfer(access locality)" + case TransferLeaseForIOOverload: + return "transfer(io-overload)" + case TransferLeaseForPreferences: + return "transfer(preferences)" + case DontTransferLeaseBalanced: + return "no-transfer(balanced)" + case DontTransferLeaseNoStoreDescriptor: + return "no-transfer(missing store descriptor)" + case DontTransferLeaseNoValidTargets: + return "no-transfer(no valid targets)" + default: + panic(fmt.Sprintf("unknown transfer lease decision %d", t)) + } +} + // ShouldTransferLease returns true if the specified store is overfull in terms // of leases with respect to the other stores matching the specified // attributes. @@ -2538,14 +2632,14 @@ func (a *Allocator) ShouldTransferLease( GetFirstIndex() kvpb.RaftIndex }, usageInfo allocator.RangeUsageInfo, -) bool { +) TransferLeaseDecision { if a.leaseholderShouldMoveDueToPreferences(ctx, storePool, conf, leaseRepl, existing) { - return true + return TransferLeaseForPreferences } if a.leaseholderShouldMoveDueToIOOverload( ctx, storePool, existing, leaseRepl.StoreID(), a.IOOverloadOptions()) { - return true + return TransferLeaseForIOOverload } existing = a.ValidLeaseTargets( @@ -2560,11 +2654,11 @@ func (a *Allocator) ShouldTransferLease( // Short-circuit if there are no valid targets out there. if len(existing) == 0 || (len(existing) == 1 && existing[0].StoreID == leaseRepl.StoreID()) { - return false + return DontTransferLeaseNoValidTargets } source, ok := storePool.GetStoreDescriptor(leaseRepl.StoreID()) if !ok { - return false + return DontTransferLeaseNoStoreDescriptor } sl, _, _ := storePool.GetStoreList(storepool.StoreFilterSuspect) @@ -2581,20 +2675,23 @@ func (a *Allocator) ShouldTransferLease( nil, sl.CandidateLeases.Mean, ) - var result bool + var result TransferLeaseDecision switch transferDec { case shouldNotTransfer: - result = false + result = DontTransferLeaseBalanced case shouldTransfer: - result = true + result = TransferLeaseForAccessLocality case decideWithoutStats: - result = a.shouldTransferLeaseForLeaseCountConvergence(ctx, storePool, sl, source, existing) + if a.shouldTransferLeaseForLeaseCountConvergence(ctx, storePool, sl, source, existing) { + result = TransferLeaseForCountBalance + } else { + result = DontTransferLeaseBalanced + } default: log.KvDistribution.Fatalf(ctx, "unexpected transfer decision %d", transferDec) } - log.KvDistribution.VEventf( - ctx, 3, "ShouldTransferLease decision (lease-holder=s%d): %t", leaseRepl.StoreID(), result, + ctx, 3, "ShouldTransferLease decision (lease-holder=s%d): %v", leaseRepl.StoreID(), result, ) return result } @@ -2634,7 +2731,7 @@ func (a Allocator) shouldTransferLeaseForAccessLocality( usageInfo allocator.RangeUsageInfo, rebalanceAdjustments map[roachpb.StoreID]float64, candidateLeasesMean float64, -) (transferDecision, roachpb.ReplicaDescriptor) { +) (accessLocalityTransferDecision, roachpb.ReplicaDescriptor) { // Only use load-based rebalancing if it's enabled and we have both // stats and locality information to base our decision on. if usageInfo.RequestLocality == nil || diff --git a/pkg/kv/kvserver/allocator/allocatorimpl/allocator_test.go b/pkg/kv/kvserver/allocator/allocatorimpl/allocator_test.go index 5541b894e361..3a3a7a3c7c0c 100644 --- a/pkg/kv/kvserver/allocator/allocatorimpl/allocator_test.go +++ b/pkg/kv/kvserver/allocator/allocatorimpl/allocator_test.go @@ -2682,21 +2682,21 @@ func TestAllocatorShouldTransferLease(t *testing.T) { testCases := []struct { leaseholder roachpb.StoreID existing []roachpb.ReplicaDescriptor - expected bool + expected TransferLeaseDecision }{ - {leaseholder: 1, existing: nil, expected: false}, - {leaseholder: 2, existing: nil, expected: false}, - {leaseholder: 3, existing: nil, expected: false}, - {leaseholder: 4, existing: nil, expected: false}, - {leaseholder: 3, existing: replicas(1), expected: true}, - {leaseholder: 3, existing: replicas(1, 2), expected: true}, - {leaseholder: 3, existing: replicas(2), expected: false}, - {leaseholder: 3, existing: replicas(3), expected: false}, - {leaseholder: 3, existing: replicas(4), expected: false}, - {leaseholder: 4, existing: replicas(1), expected: true}, - {leaseholder: 4, existing: replicas(2), expected: true}, - {leaseholder: 4, existing: replicas(3), expected: true}, - {leaseholder: 4, existing: replicas(1, 2, 3), expected: true}, + {leaseholder: 1, existing: nil, expected: DontTransferLeaseNoValidTargets}, + {leaseholder: 2, existing: nil, expected: DontTransferLeaseNoValidTargets}, + {leaseholder: 3, existing: nil, expected: DontTransferLeaseNoValidTargets}, + {leaseholder: 4, existing: nil, expected: DontTransferLeaseNoValidTargets}, + {leaseholder: 3, existing: replicas(1), expected: TransferLeaseForCountBalance}, + {leaseholder: 3, existing: replicas(1, 2), expected: TransferLeaseForCountBalance}, + {leaseholder: 3, existing: replicas(2), expected: DontTransferLeaseBalanced}, + {leaseholder: 3, existing: replicas(3), expected: DontTransferLeaseNoValidTargets}, + {leaseholder: 3, existing: replicas(4), expected: DontTransferLeaseBalanced}, + {leaseholder: 4, existing: replicas(1), expected: TransferLeaseForCountBalance}, + {leaseholder: 4, existing: replicas(2), expected: TransferLeaseForCountBalance}, + {leaseholder: 4, existing: replicas(3), expected: TransferLeaseForCountBalance}, + {leaseholder: 4, existing: replicas(1, 2, 3), expected: TransferLeaseForCountBalance}, } for _, c := range testCases { t.Run("", func(t *testing.T) { @@ -2752,20 +2752,20 @@ func TestAllocatorShouldTransferLeaseDraining(t *testing.T) { testCases := []struct { leaseholder roachpb.StoreID existing []roachpb.ReplicaDescriptor - expected bool + expected TransferLeaseDecision }{ - {leaseholder: 1, existing: nil, expected: false}, - {leaseholder: 2, existing: nil, expected: false}, - {leaseholder: 3, existing: nil, expected: false}, - {leaseholder: 4, existing: nil, expected: false}, - {leaseholder: 2, existing: replicas(1), expected: false}, - {leaseholder: 3, existing: replicas(1), expected: false}, - {leaseholder: 3, existing: replicas(1, 2), expected: false}, - {leaseholder: 3, existing: replicas(1, 2, 4), expected: false}, - {leaseholder: 4, existing: replicas(1), expected: false}, - {leaseholder: 4, existing: replicas(1, 2), expected: true}, - {leaseholder: 4, existing: replicas(1, 3), expected: true}, - {leaseholder: 4, existing: replicas(1, 2, 3), expected: true}, + {leaseholder: 1, existing: nil, expected: DontTransferLeaseNoValidTargets}, + {leaseholder: 2, existing: nil, expected: DontTransferLeaseNoValidTargets}, + {leaseholder: 3, existing: nil, expected: DontTransferLeaseNoValidTargets}, + {leaseholder: 4, existing: nil, expected: DontTransferLeaseNoValidTargets}, + {leaseholder: 2, existing: replicas(1), expected: DontTransferLeaseNoValidTargets}, + {leaseholder: 3, existing: replicas(1), expected: DontTransferLeaseNoValidTargets}, + {leaseholder: 3, existing: replicas(1, 2), expected: DontTransferLeaseBalanced}, + {leaseholder: 3, existing: replicas(1, 2, 4), expected: DontTransferLeaseBalanced}, + {leaseholder: 4, existing: replicas(1), expected: DontTransferLeaseNoValidTargets}, + {leaseholder: 4, existing: replicas(1, 2), expected: TransferLeaseForCountBalance}, + {leaseholder: 4, existing: replicas(1, 3), expected: TransferLeaseForCountBalance}, + {leaseholder: 4, existing: replicas(1, 2, 3), expected: TransferLeaseForCountBalance}, } for _, c := range testCases { t.Run("", func(t *testing.T) { @@ -2826,7 +2826,7 @@ func TestAllocatorShouldTransferSuspected(t *testing.T) { &mockRepl{storeID: 2, replicationFactor: 3}, allocator.RangeUsageInfo{}, ) - require.Equal(t, expected, result) + require.Equal(t, expected, result.ShouldTransfer()) } timeAfterNodeSuspect := liveness.TimeAfterNodeSuspect.Get(&a.st.SV) // Based on capacity node 1 is desirable. @@ -2861,7 +2861,7 @@ func TestAllocatorShouldTransferLeaseIOOverload(t *testing.T) { leaseCounts, IOScores []float64 leaseholder roachpb.StoreID excludeLeaseRepl bool - expected bool + expected TransferLeaseDecision enforcement IOOverloadEnforcementLevel }{ { @@ -2869,7 +2869,7 @@ func TestAllocatorShouldTransferLeaseIOOverload(t *testing.T) { leaseCounts: floats(100, 100, 100, 100, 100), IOScores: floats(2.5, 1.5, 0.5, 0, 0), leaseholder: 1, - expected: false, + expected: DontTransferLeaseBalanced, enforcement: IOOverloadThresholdBlockTransfers, }, { @@ -2879,7 +2879,7 @@ func TestAllocatorShouldTransferLeaseIOOverload(t *testing.T) { leaseholder: 1, // Store 3 is above the threshold (1.0 > 0.8), but equal to the avg (1.0), so // it is still considered a non-IO-overloaded candidate. - expected: true, + expected: TransferLeaseForIOOverload, enforcement: IOOverloadThresholdShed, }, { @@ -2887,7 +2887,7 @@ func TestAllocatorShouldTransferLeaseIOOverload(t *testing.T) { leaseCounts: floats(0, 100, 100, 400, 400), IOScores: floats(2.5, 1.5, 0.5, 0, 0), leaseholder: 5, - expected: true, + expected: TransferLeaseForCountBalance, enforcement: IOOverloadThresholdIgnore, }, { @@ -2895,7 +2895,7 @@ func TestAllocatorShouldTransferLeaseIOOverload(t *testing.T) { leaseCounts: floats(0, 0, 0, 0, 0), IOScores: floats(0.89, 0, 0, 0, 0), leaseholder: 1, - expected: false, + expected: DontTransferLeaseBalanced, enforcement: IOOverloadThresholdShed, }, } @@ -3006,54 +3006,55 @@ func TestAllocatorLeasePreferences(t *testing.T) { preferences []roachpb.LeasePreference expectAllowLeaseRepl roachpb.StoreID /* excludeLeaseRepl = false */ expectExcludeLeaseRepl roachpb.StoreID /* excludeLeaseRepl = true */ + expectShouldTransfer TransferLeaseDecision }{ - {1, nil, preferDC1, 0, 0}, - {1, replicas(1, 2, 3, 4), preferDC1, 0, 2}, - {1, replicas(2, 3, 4), preferDC1, 0, 2}, - {2, replicas(1, 2, 3, 4), preferDC1, 1, 1}, - {2, replicas(2, 3, 4), preferDC1, 0, 3}, - {4, replicas(2, 3, 4), preferDC1, 2, 2}, - {1, nil, preferDC4Then3Then2, 0, 0}, - {1, replicas(1, 2, 3, 4), preferDC4Then3Then2, 4, 4}, - {1, replicas(1, 2, 3), preferDC4Then3Then2, 3, 3}, - {1, replicas(1, 2), preferDC4Then3Then2, 2, 2}, - {3, replicas(1, 2, 3, 4), preferDC4Then3Then2, 4, 4}, - {3, replicas(1, 2, 3), preferDC4Then3Then2, 0, 2}, - {3, replicas(1, 3), preferDC4Then3Then2, 0, 1}, - {4, replicas(1, 2, 3, 4), preferDC4Then3Then2, 0, 3}, - {4, replicas(1, 2, 4), preferDC4Then3Then2, 0, 2}, - {4, replicas(1, 4), preferDC4Then3Then2, 0, 1}, - {1, replicas(1, 2, 3, 4), preferN2ThenS3, 2, 2}, - {1, replicas(1, 3, 4), preferN2ThenS3, 3, 3}, - {1, replicas(1, 4), preferN2ThenS3, 0, 4}, - {2, replicas(1, 2, 3, 4), preferN2ThenS3, 0, 3}, - {2, replicas(1, 2, 4), preferN2ThenS3, 0, 1}, - {3, replicas(1, 2, 3, 4), preferN2ThenS3, 2, 2}, - {3, replicas(1, 3, 4), preferN2ThenS3, 0, 1}, - {4, replicas(1, 4), preferN2ThenS3, 1, 1}, - {1, replicas(1, 2, 3, 4), preferNotS1ThenNotN2, 2, 2}, - {1, replicas(1, 3, 4), preferNotS1ThenNotN2, 3, 3}, - {1, replicas(1, 2), preferNotS1ThenNotN2, 2, 2}, - {1, replicas(1), preferNotS1ThenNotN2, 0, 0}, - {2, replicas(1, 2, 3, 4), preferNotS1ThenNotN2, 0, 3}, - {2, replicas(2, 3, 4), preferNotS1ThenNotN2, 0, 3}, - {2, replicas(1, 2, 3), preferNotS1ThenNotN2, 0, 3}, - {2, replicas(1, 2, 4), preferNotS1ThenNotN2, 0, 4}, - {4, replicas(1, 2, 3, 4), preferNotS1ThenNotN2, 2, 2}, - {4, replicas(1, 4), preferNotS1ThenNotN2, 0, 1}, - {1, replicas(1, 2, 3, 4), preferNotS1AndNotN2, 3, 3}, - {1, replicas(1, 2), preferNotS1AndNotN2, 0, 2}, - {2, replicas(1, 2, 3, 4), preferNotS1AndNotN2, 3, 3}, - {2, replicas(2, 3, 4), preferNotS1AndNotN2, 3, 3}, - {2, replicas(1, 2, 3), preferNotS1AndNotN2, 3, 3}, - {2, replicas(1, 2, 4), preferNotS1AndNotN2, 4, 4}, - {3, replicas(1, 3), preferNotS1AndNotN2, 0, 1}, - {4, replicas(1, 4), preferNotS1AndNotN2, 0, 1}, - {1, replicas(1, 2, 3, 4), preferMatchesNothing, 0, 2}, - {2, replicas(1, 2, 3, 4), preferMatchesNothing, 0, 1}, - {3, replicas(1, 3, 4), preferMatchesNothing, 1, 1}, - {4, replicas(1, 3, 4), preferMatchesNothing, 1, 1}, - {4, replicas(2, 3, 4), preferMatchesNothing, 2, 2}, + {1, nil, preferDC1, 0, 0, DontTransferLeaseNoValidTargets}, + {1, replicas(1, 2, 3, 4), preferDC1, 0, 2, DontTransferLeaseNoValidTargets}, + {1, replicas(2, 3, 4), preferDC1, 0, 2, DontTransferLeaseBalanced}, + {2, replicas(1, 2, 3, 4), preferDC1, 1, 1, TransferLeaseForPreferences}, + {2, replicas(2, 3, 4), preferDC1, 0, 3, DontTransferLeaseBalanced}, + {4, replicas(2, 3, 4), preferDC1, 2, 2, TransferLeaseForCountBalance}, + {1, nil, preferDC4Then3Then2, 0, 0, DontTransferLeaseNoValidTargets}, + {1, replicas(1, 2, 3, 4), preferDC4Then3Then2, 4, 4, TransferLeaseForPreferences}, + {1, replicas(1, 2, 3), preferDC4Then3Then2, 3, 3, TransferLeaseForPreferences}, + {1, replicas(1, 2), preferDC4Then3Then2, 2, 2, TransferLeaseForPreferences}, + {3, replicas(1, 2, 3, 4), preferDC4Then3Then2, 4, 4, TransferLeaseForPreferences}, + {3, replicas(1, 2, 3), preferDC4Then3Then2, 0, 2, DontTransferLeaseNoValidTargets}, + {3, replicas(1, 3), preferDC4Then3Then2, 0, 1, DontTransferLeaseNoValidTargets}, + {4, replicas(1, 2, 3, 4), preferDC4Then3Then2, 0, 3, DontTransferLeaseNoValidTargets}, + {4, replicas(1, 2, 4), preferDC4Then3Then2, 0, 2, DontTransferLeaseNoValidTargets}, + {4, replicas(1, 4), preferDC4Then3Then2, 0, 1, DontTransferLeaseNoValidTargets}, + {1, replicas(1, 2, 3, 4), preferN2ThenS3, 2, 2, TransferLeaseForPreferences}, + {1, replicas(1, 3, 4), preferN2ThenS3, 3, 3, TransferLeaseForPreferences}, + {1, replicas(1, 4), preferN2ThenS3, 0, 4, DontTransferLeaseBalanced}, + {2, replicas(1, 2, 3, 4), preferN2ThenS3, 0, 3, DontTransferLeaseNoValidTargets}, + {2, replicas(1, 2, 4), preferN2ThenS3, 0, 1, DontTransferLeaseNoValidTargets}, + {3, replicas(1, 2, 3, 4), preferN2ThenS3, 2, 2, TransferLeaseForPreferences}, + {3, replicas(1, 3, 4), preferN2ThenS3, 0, 1, DontTransferLeaseNoValidTargets}, + {4, replicas(1, 4), preferN2ThenS3, 1, 1, TransferLeaseForCountBalance}, + {1, replicas(1, 2, 3, 4), preferNotS1ThenNotN2, 2, 2, TransferLeaseForPreferences}, + {1, replicas(1, 3, 4), preferNotS1ThenNotN2, 3, 3, TransferLeaseForPreferences}, + {1, replicas(1, 2), preferNotS1ThenNotN2, 2, 2, TransferLeaseForPreferences}, + {1, replicas(1), preferNotS1ThenNotN2, 0, 0, DontTransferLeaseNoValidTargets}, + {2, replicas(1, 2, 3, 4), preferNotS1ThenNotN2, 0, 3, DontTransferLeaseBalanced}, + {2, replicas(2, 3, 4), preferNotS1ThenNotN2, 0, 3, DontTransferLeaseBalanced}, + {2, replicas(1, 2, 3), preferNotS1ThenNotN2, 0, 3, DontTransferLeaseBalanced}, + {2, replicas(1, 2, 4), preferNotS1ThenNotN2, 0, 4, DontTransferLeaseBalanced}, + {4, replicas(1, 2, 3, 4), preferNotS1ThenNotN2, 2, 2, TransferLeaseForCountBalance}, + {4, replicas(1, 4), preferNotS1ThenNotN2, 0, 1, DontTransferLeaseNoValidTargets}, + {1, replicas(1, 2, 3, 4), preferNotS1AndNotN2, 3, 3, TransferLeaseForPreferences}, + {1, replicas(1, 2), preferNotS1AndNotN2, 0, 2, DontTransferLeaseBalanced}, + {2, replicas(1, 2, 3, 4), preferNotS1AndNotN2, 3, 3, TransferLeaseForPreferences}, + {2, replicas(2, 3, 4), preferNotS1AndNotN2, 3, 3, TransferLeaseForPreferences}, + {2, replicas(1, 2, 3), preferNotS1AndNotN2, 3, 3, TransferLeaseForPreferences}, + {2, replicas(1, 2, 4), preferNotS1AndNotN2, 4, 4, TransferLeaseForPreferences}, + {3, replicas(1, 3), preferNotS1AndNotN2, 0, 1, DontTransferLeaseNoValidTargets}, + {4, replicas(1, 4), preferNotS1AndNotN2, 0, 1, DontTransferLeaseNoValidTargets}, + {1, replicas(1, 2, 3, 4), preferMatchesNothing, 0, 2, DontTransferLeaseBalanced}, + {2, replicas(1, 2, 3, 4), preferMatchesNothing, 0, 1, DontTransferLeaseBalanced}, + {3, replicas(1, 3, 4), preferMatchesNothing, 1, 1, TransferLeaseForCountBalance}, + {4, replicas(1, 3, 4), preferMatchesNothing, 1, 1, TransferLeaseForCountBalance}, + {4, replicas(2, 3, 4), preferMatchesNothing, 2, 2, TransferLeaseForCountBalance}, } for _, c := range testCases { @@ -3071,9 +3072,8 @@ func TestAllocatorLeasePreferences(t *testing.T) { }, allocator.RangeUsageInfo{}, ) - expectTransfer := c.expectAllowLeaseRepl != 0 - if expectTransfer != result { - t.Errorf("expected %v, but found %v", expectTransfer, result) + if c.expectShouldTransfer != result { + t.Errorf("expected %v, but found %v", c.expectShouldTransfer, result) } target := a.TransferLeaseTarget( ctx, diff --git a/pkg/kv/kvserver/allocator/plan/replicate.go b/pkg/kv/kvserver/allocator/plan/replicate.go index d9acac3218c5..c623c2a0aa6b 100644 --- a/pkg/kv/kvserver/allocator/plan/replicate.go +++ b/pkg/kv/kvserver/allocator/plan/replicate.go @@ -204,8 +204,8 @@ func (rp ReplicaPlanner) ShouldPlanChange( } // If the lease is valid, check to see if we should transfer it. - if canTransferLeaseFrom(ctx, repl, conf) && - rp.allocator.ShouldTransferLease( + if canTransferLeaseFrom(ctx, repl, conf) { + decision := rp.allocator.ShouldTransferLease( ctx, rp.storePool, desc, @@ -213,9 +213,12 @@ func (rp ReplicaPlanner) ShouldPlanChange( voterReplicas, repl, repl.RangeUsageInfo(), - ) { - log.KvDistribution.VEventf(ctx, 2, "lease transfer needed, enqueuing") - return true, 0 + ) + if decision.ShouldTransfer() { + log.KvDistribution.VEventf(ctx, + 2, "lease transfer needed %v, enqueuing", decision) + return true, 0 + } } leaseStatus := repl.LeaseStatusAt(ctx, now) diff --git a/pkg/testutils/lint/passes/redactcheck/redactcheck.go b/pkg/testutils/lint/passes/redactcheck/redactcheck.go index 3f5c0605ce30..b0b4716e3fa3 100644 --- a/pkg/testutils/lint/passes/redactcheck/redactcheck.go +++ b/pkg/testutils/lint/passes/redactcheck/redactcheck.go @@ -93,9 +93,10 @@ func runAnalyzer(pass *analysis.Pass) (interface{}, error) { "LeaseTransferOutcome": {}, }, "github.com/cockroachdb/cockroach/pkg/kv/kvserver/allocator/allocatorimpl": { - "AllocatorAction": {}, - "TargetReplicaType": {}, - "ReplicaStatus": {}, + "AllocatorAction": {}, + "TargetReplicaType": {}, + "ReplicaStatus": {}, + "TransferLeaseDecision": {}, }, "github.com/cockroachdb/cockroach/pkg/kv/kvserver/allocator/load": { "Dimension": {},