shouldn't try leader first when leader's store is slow in prefer-lead…

…er strategy in replicaSelectorV2 (tikv#1215) * shouldn't try leader first when leader's store is slow in prefer-leader strategy in replicaSelectorV2 Signed-off-by: crazycs520 <[email protected]> * refine comment Signed-off-by: crazycs520 <[email protected]> * refine comment Signed-off-by: crazycs520 <[email protected]> * fix bug and add test Signed-off-by: crazycs520 <[email protected]> --------- Signed-off-by: crazycs520 <[email protected]>
pingyu · Mar 12, 2024 · 0416875 · 0416875
1 parent 3e419e6
commit 0416875
Show file tree

Hide file tree

Showing 2 changed files with 102 additions and 16 deletions.
diff --git a/internal/locate/replica_selector.go b/internal/locate/replica_selector.go
@@ -298,6 +298,11 @@ func (s *ReplicaSelectMixedStrategy) isCandidate(r *replica, isLeader bool, epoc
 	if s.busyThreshold > 0 && (r.store.EstimatedWaitTime() > s.busyThreshold || r.serverIsBusy || isLeader) {
 		return false
 	}
+	if s.preferLeader && r.store.healthStatus.IsSlow() && !isLeader {
+		// This logic is used to compatible with old replica selector.
+		// When use prefer-leader strategy, if the replica is slow, and it's not leader, then it is not candidate, skip it.
+		return false
+	}
 	return true
 }
 
@@ -320,7 +325,13 @@ func (s *ReplicaSelectMixedStrategy) calculateScore(r *replica, isLeader bool) i
 	}
 	if isLeader {
 		if s.preferLeader {
-			score |= flagPreferLeader
+			// Following logic is used to compatible with old replica selector.
+			// When use prefer-leader strategy, only prefer leader when the leader's store is not slow.
+			if !r.store.healthStatus.IsSlow() {
+				score |= flagPreferLeader
+			} else {
+				score |= flagNormalPeer
+			}
 		} else if s.tryLeader {
 			if len(s.labels) > 0 {
 				// When the leader has matching labels, prefer leader than other mismatching peers.

diff --git a/internal/locate/replica_selector_test.go b/internal/locate/replica_selector_test.go
@@ -180,11 +180,7 @@ func TestReplicaSelectorCalculateScore(t *testing.T) {
 		s.Equal(score, flagLabelMatches+flagNormalPeer+flagNotAttempt)
 		strategy.preferLeader = true
 		score = strategy.calculateScore(r, isLeader)
-		if isLeader {
-			s.Equal(score, flagLabelMatches+flagPreferLeader+flagNotAttempt)
-		} else {
-			s.Equal(score, flagLabelMatches+flagNormalPeer+flagNotAttempt)
-		}
+		s.Equal(score, flagLabelMatches+flagNormalPeer+flagNotAttempt)
 		strategy.learnerOnly = true
 		strategy.tryLeader = false
 		strategy.preferLeader = false
@@ -218,18 +214,10 @@ func TestReplicaSelectorCalculateScore(t *testing.T) {
 			labels:       labels,
 		}
 		score = strategy.calculateScore(r, isLeader)
-		if isLeader {
-			s.Equal(score, flagPreferLeader+flagNotAttempt)
-		} else {
-			s.Equal(score, flagNormalPeer+flagNotAttempt)
-		}
+		s.Equal(score, flagNormalPeer+flagNotAttempt)
 		r.store.labels = labels
 		score = strategy.calculateScore(r, isLeader)
-		if isLeader {
-			s.Equal(score, flagLabelMatches+flagPreferLeader+flagNotAttempt)
-		} else {
-			s.Equal(score, flagLabelMatches+flagNormalPeer+flagNotAttempt)
-		}
+		s.Equal(score, flagLabelMatches+flagNormalPeer+flagNotAttempt)
 		r.store.labels = nil
 	}
 }
@@ -1658,6 +1646,93 @@ func TestReplicaReadAccessPathByMixedAndPreferLeaderCase(t *testing.T) {
 	}
 	s.True(s.runCaseAndCompare(ca))
 	s.changeRegionLeader(1)
+
+	cas := []replicaSelectorAccessPathCase{
+		{
+			reqType:   tikvrpc.CmdGet,
+			readType:  kv.ReplicaReadPreferLeader,
+			staleRead: false,
+			timeout:   0,
+			accessErr: []RegionErrorType{ServerIsBusyErr},
+			expect: &accessPathResult{
+				accessPath: []string{
+					"{addr: store1, replica-read: true, stale-read: false}", // store1 will be marked already slow.
+					"{addr: store2, replica-read: true, stale-read: false}",
+				},
+				respErr:         "",
+				respRegionError: nil,
+				backoffCnt:      0,
+				backoffDetail:   []string{},
+				regionIsValid:   true,
+			},
+			afterRun: func() { /* don't invalid region */ },
+		},
+		{
+			reqType:   tikvrpc.CmdGet,
+			readType:  kv.ReplicaReadPreferLeader,
+			staleRead: false,
+			timeout:   0,
+			accessErr: []RegionErrorType{ServerIsBusyErr, ServerIsBusyErr, ServerIsBusyErr},
+			beforeRun: func() { /* don't resetStoreState */ },
+			expect: &accessPathResult{
+				accessPath: []string{
+					"{addr: store2, replica-read: true, stale-read: false}", // won't try leader in store1, since it is slow.
+					"{addr: store3, replica-read: true, stale-read: false}",
+					"{addr: store1, replica-read: true, stale-read: false}",
+				},
+				respErr:         "",
+				respRegionError: fakeEpochNotMatch,
+				backoffCnt:      1,
+				backoffDetail:   []string{"tikvServerBusy+1"},
+				regionIsValid:   false,
+			},
+		},
+	}
+	s.True(s.runMultiCaseAndCompare(cas))
+
+	cas = []replicaSelectorAccessPathCase{
+		{
+			reqType:   tikvrpc.CmdGet,
+			readType:  kv.ReplicaReadPreferLeader,
+			staleRead: false,
+			timeout:   0,
+			accessErr: []RegionErrorType{ServerIsBusyErr, ServerIsBusyErr},
+			expect: &accessPathResult{
+				accessPath: []string{
+					"{addr: store1, replica-read: true, stale-read: false}", // store1 will be marked already slow.
+					"{addr: store2, replica-read: true, stale-read: false}", // store2 will be marked already slow.
+					"{addr: store3, replica-read: true, stale-read: false}",
+				},
+				respErr:         "",
+				respRegionError: nil,
+				backoffCnt:      0,
+				backoffDetail:   []string{},
+				regionIsValid:   true,
+			},
+			afterRun: func() { /* don't invalid region */ },
+		},
+		{
+			reqType:   tikvrpc.CmdGet,
+			readType:  kv.ReplicaReadPreferLeader,
+			staleRead: false,
+			timeout:   0,
+			accessErr: []RegionErrorType{ServerIsBusyErr, ServerIsBusyErr, ServerIsBusyErr},
+			beforeRun: func() { /* don't resetStoreState */ },
+			expect: &accessPathResult{
+				accessPath: []string{
+					"{addr: store3, replica-read: true, stale-read: false}", // won't try leader in store1, since it is slow, ditto for store2.
+					"{addr: store1, replica-read: true, stale-read: false}",
+					// won't retry store2, since it is slow, and it is not leader replica.
+				},
+				respErr:         "",
+				respRegionError: fakeEpochNotMatch,
+				backoffCnt:      1,
+				backoffDetail:   []string{"tikvServerBusy+1"},
+				regionIsValid:   false,
+			},
+		},
+	}
+	s.True(s.runMultiCaseAndCompare(cas))
 }
 
 func TestReplicaReadAccessPathByStaleReadCase(t *testing.T) {