From 2a92b1a06692a087b5d1aa153c2f15b30ac94cd8 Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Tue, 27 Feb 2024 09:01:57 +0800 Subject: [PATCH 01/14] reduce unnecessary tikvServerBusy backoff when Signed-off-by: crazycs520 --- internal/locate/region_request.go | 16 ++++----- internal/locate/replica_selector_test.go | 41 ++++++++++++++++++------ 2 files changed, 37 insertions(+), 20 deletions(-) diff --git a/internal/locate/region_request.go b/internal/locate/region_request.go index 277899d68..7a14ebe0d 100644 --- a/internal/locate/region_request.go +++ b/internal/locate/region_request.go @@ -1277,9 +1277,9 @@ func (s *replicaSelector) onServerIsBusy( // Mark the server is busy (the next incoming READs could be redirect // to expected followers. ) ctx.Store.markAlreadySlow() - if s.canFallback2Follower() { - return true, nil - } + } + if s.canFallback2Follower() { + return true, nil } err = bo.Backoff(retry.BoTiKVServerBusy, errors.Errorf("server is busy, ctx: %v", ctx)) if err != nil { @@ -1294,15 +1294,11 @@ func (s *replicaSelector) canFallback2Follower() bool { if s == nil || s.state == nil { return false } - state, ok := s.state.(*accessFollower) - if !ok { - return false - } - if !state.isStaleRead { + _, ok := s.state.(*accessKnownLeader) + if ok { return false } - // can fallback to follower only when the leader is exhausted. - return state.lastIdx == state.leaderIdx && state.IsLeaderExhausted(s.replicas[state.leaderIdx]) + return true } func (s *replicaSelector) onDataIsNotReady() { diff --git a/internal/locate/replica_selector_test.go b/internal/locate/replica_selector_test.go index 0df8a09e0..7703fa100 100644 --- a/internal/locate/replica_selector_test.go +++ b/internal/locate/replica_selector_test.go @@ -114,8 +114,8 @@ func TestReplicaReadAccessPathByCase(t *testing.T) { }, respErr: "", respRegionError: nil, - backoffCnt: 1, - backoffDetail: []string{"tikvServerBusy+1"}, + backoffCnt: 0, + backoffDetail: []string{}, regionIsValid: true, }, } @@ -297,8 +297,8 @@ func TestReplicaReadAccessPathByCase(t *testing.T) { "{addr: store2, replica-read: true, stale-read: false}"}, respErr: "", respRegionError: nil, - backoffCnt: 1, - backoffDetail: []string{"tikvServerBusy+1"}, + backoffCnt: 0, + backoffDetail: []string{}, regionIsValid: true, }, } @@ -319,8 +319,8 @@ func TestReplicaReadAccessPathByCase(t *testing.T) { }, respErr: "", respRegionError: fakeEpochNotMatch, - backoffCnt: 2, - backoffDetail: []string{"tikvServerBusy+2"}, + backoffCnt: 0, + backoffDetail: []string{}, regionIsValid: false, }, } @@ -341,8 +341,8 @@ func TestReplicaReadAccessPathByCase(t *testing.T) { }, respErr: "", respRegionError: fakeEpochNotMatch, - backoffCnt: 2, - backoffDetail: []string{"tikvServerBusy+2"}, + backoffCnt: 0, + backoffDetail: []string{}, regionIsValid: false, }, } @@ -363,8 +363,29 @@ func TestReplicaReadAccessPathByCase(t *testing.T) { "{addr: store3, replica-read: true, stale-read: false}"}, respErr: "", respRegionError: nil, - backoffCnt: 1, - backoffDetail: []string{"tikvServerBusy+1"}, + backoffCnt: 0, + backoffDetail: []string{}, + regionIsValid: true, + }, + } + s.True(s.runCaseAndCompare(ca)) + + ca = replicaSelectorAccessPathCase{ + reqType: tikvrpc.CmdGet, + readType: kv.ReplicaReadMixed, + staleRead: true, + timeout: time.Microsecond * 100, + accessErr: []RegionErrorType{ServerIsBusyErr, ServerIsBusyWithEstimatedWaitMsErr}, + expect: &accessPathResult{ + accessPath: []string{ + "{addr: store1, replica-read: false, stale-read: true}", + "{addr: store2, replica-read: true, stale-read: false}", + "{addr: store3, replica-read: true, stale-read: false}", + }, + respErr: "", + respRegionError: nil, + backoffCnt: 0, + backoffDetail: []string{}, regionIsValid: true, }, } From 6439bf054f04819f3f9db6a4ca6fc54562556c23 Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Tue, 27 Feb 2024 09:10:48 +0800 Subject: [PATCH 02/14] fix lint Signed-off-by: crazycs520 --- internal/locate/region_request.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/internal/locate/region_request.go b/internal/locate/region_request.go index 7a14ebe0d..7dd5a24f8 100644 --- a/internal/locate/region_request.go +++ b/internal/locate/region_request.go @@ -1295,10 +1295,7 @@ func (s *replicaSelector) canFallback2Follower() bool { return false } _, ok := s.state.(*accessKnownLeader) - if ok { - return false - } - return true + return !ok } func (s *replicaSelector) onDataIsNotReady() { From d393317d786c74b1b01cd6016f00ff5a29255653 Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Tue, 27 Feb 2024 15:41:57 +0800 Subject: [PATCH 03/14] refine code and add test Signed-off-by: crazycs520 --- internal/locate/region_request.go | 22 ++++++--- internal/locate/replica_selector_test.go | 61 ++++++++++++++++++++++-- 2 files changed, 73 insertions(+), 10 deletions(-) diff --git a/internal/locate/region_request.go b/internal/locate/region_request.go index 7dd5a24f8..e58f6fbf8 100644 --- a/internal/locate/region_request.go +++ b/internal/locate/region_request.go @@ -1278,7 +1278,7 @@ func (s *replicaSelector) onServerIsBusy( // to expected followers. ) ctx.Store.markAlreadySlow() } - if s.canFallback2Follower() { + if s.canSkipServerIsBusyBackoff() { return true, nil } err = bo.Backoff(retry.BoTiKVServerBusy, errors.Errorf("server is busy, ctx: %v", ctx)) @@ -1288,14 +1288,24 @@ func (s *replicaSelector) onServerIsBusy( return true, nil } -// For some reasons, the leader is unreachable by now, try followers instead. -// the state is changed in accessFollower.next when leader is unavailable. -func (s *replicaSelector) canFallback2Follower() bool { +// canSkipServerIsBusyBackoff returns true if the request can be sent to next replica and can skip ServerIsBusy backoff. +func (s *replicaSelector) canSkipServerIsBusyBackoff() bool { if s == nil || s.state == nil { return false } - _, ok := s.state.(*accessKnownLeader) - return !ok + accessLeader, ok := s.state.(*accessKnownLeader) + if ok && accessLeader.isCandidate(s.replicas[accessLeader.leaderIdx]) { + // If leader is still candidate, the request will be sent to leader again, + // so don't skip since the leader is still busy. + return false + } + for _, replica := range s.replicas { + if replica.attempts == 0 && replica.store.getLivenessState() == reachable && + !replica.isEpochStale() && !replica.store.isSlow() { + return true + } + } + return false } func (s *replicaSelector) onDataIsNotReady() { diff --git a/internal/locate/replica_selector_test.go b/internal/locate/replica_selector_test.go index 7703fa100..99a5b0094 100644 --- a/internal/locate/replica_selector_test.go +++ b/internal/locate/replica_selector_test.go @@ -319,8 +319,8 @@ func TestReplicaReadAccessPathByCase(t *testing.T) { }, respErr: "", respRegionError: fakeEpochNotMatch, - backoffCnt: 0, - backoffDetail: []string{}, + backoffCnt: 1, + backoffDetail: []string{"tikvServerBusy+1"}, regionIsValid: false, }, } @@ -341,8 +341,8 @@ func TestReplicaReadAccessPathByCase(t *testing.T) { }, respErr: "", respRegionError: fakeEpochNotMatch, - backoffCnt: 0, - backoffDetail: []string{}, + backoffCnt: 2, + backoffDetail: []string{"tikvServerBusy+2"}, regionIsValid: false, }, } @@ -392,6 +392,59 @@ func TestReplicaReadAccessPathByCase(t *testing.T) { s.True(s.runCaseAndCompare(ca)) } +func TestCanSkipServerIsBusyBackoff(t *testing.T) { + s := new(testReplicaSelectorSuite) + s.SetupTest(t) + defer s.TearDownTest() + + loc, err := s.cache.LocateKey(s.bo, []byte("key")) + s.Nil(err) + req := tikvrpc.NewRequest(tikvrpc.CmdGet, &kvrpcpb.GetRequest{Key: []byte("key")}) + req.EnableStaleWithMixedReplicaRead() + selector, err := newReplicaSelector(s.cache, loc.Region, req) + s.Nil(err) + for i := 0; i < 3; i++ { + _, err = selector.next(s.bo) + s.Nil(err) + skip := selector.canSkipServerIsBusyBackoff() + if i < 2 { + s.True(skip) + } else { + s.False(skip) + } + } + + selector, err = newReplicaSelector(s.cache, loc.Region, req) + s.Nil(err) + _, err = selector.next(s.bo) + s.Nil(err) + // mock all replica's store is unreachable. + for _, replica := range selector.replicas { + atomic.StoreUint32(&replica.store.livenessState, uint32(unreachable)) + } + skip := selector.canSkipServerIsBusyBackoff() + s.False(skip) // can't skip since no replica is available. + refreshLivenessStates(selector.regionStore) + + // Test for leader read. + req = tikvrpc.NewRequest(tikvrpc.CmdGet, &kvrpcpb.GetRequest{Key: []byte("key")}) + req.ReplicaReadType = kv.ReplicaReadLeader + selector, err = newReplicaSelector(s.cache, loc.Region, req) + s.Nil(err) + for i := 0; i < 12; i++ { + _, err = selector.next(s.bo) + s.Nil(err) + skip := selector.canSkipServerIsBusyBackoff() + if i <= 8 { + s.False(skip) // can't skip since leader is available. + } else if i <= 10 { + s.True(skip) + } else { + s.False(skip) + } + } +} + func (s *testReplicaSelectorSuite) changeRegionLeader(storeId uint64) { loc, err := s.cache.LocateKey(s.bo, []byte("key")) s.Nil(err) From 0ffcce39ff1ae8ef1d0869789079aaa26d630f16 Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Tue, 27 Feb 2024 15:46:56 +0800 Subject: [PATCH 04/14] refine code Signed-off-by: crazycs520 --- internal/locate/region_request.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/internal/locate/region_request.go b/internal/locate/region_request.go index e58f6fbf8..035d54583 100644 --- a/internal/locate/region_request.go +++ b/internal/locate/region_request.go @@ -1265,12 +1265,6 @@ func (s *replicaSelector) onServerIsBusy( // Clear attempt history of the leader, so the leader can be accessed again. s.replicas[state.leaderIdx].attempts = 0 s.state = &tryIdleReplica{leaderIdx: state.leaderIdx} - return true, nil - case *tryIdleReplica: - if s.targetIdx != state.leaderIdx { - return true, nil - } - // backoff if still receiving ServerIsBusy after accessing leader again } } } else if ctx != nil && ctx.Store != nil { From 79ec04746dbba46087b0555de7c4e1fb1c42e1d5 Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Tue, 27 Feb 2024 15:50:16 +0800 Subject: [PATCH 05/14] add test Signed-off-by: crazycs520 --- internal/locate/region_request3_test.go | 6 ++++++ internal/locate/replica_selector_test.go | 11 +++++++++++ 2 files changed, 17 insertions(+) diff --git a/internal/locate/region_request3_test.go b/internal/locate/region_request3_test.go index 73008a522..400983fe2 100644 --- a/internal/locate/region_request3_test.go +++ b/internal/locate/region_request3_test.go @@ -373,6 +373,12 @@ func refreshLivenessStates(regionStore *regionStore) { } } +func refreshStoreScores(regionStore *regionStore) { + for _, store := range regionStore.stores { + store.slowScore.resetSlowScore() + } +} + func AssertRPCCtxEqual(s *testRegionRequestToThreeStoresSuite, rpcCtx *RPCContext, target *replica, proxy *replica) { s.Equal(rpcCtx.Store, target.store) s.Equal(rpcCtx.Peer, target.peer) diff --git a/internal/locate/replica_selector_test.go b/internal/locate/replica_selector_test.go index 99a5b0094..476beda38 100644 --- a/internal/locate/replica_selector_test.go +++ b/internal/locate/replica_selector_test.go @@ -425,6 +425,17 @@ func TestCanSkipServerIsBusyBackoff(t *testing.T) { skip := selector.canSkipServerIsBusyBackoff() s.False(skip) // can't skip since no replica is available. refreshLivenessStates(selector.regionStore) + skip = selector.canSkipServerIsBusyBackoff() + s.True(skip) + // mock all replica's store is slow. + for _, replica := range selector.replicas { + replica.store.markAlreadySlow() + } + skip = selector.canSkipServerIsBusyBackoff() + s.False(skip) + refreshStoreScores(selector.regionStore) + skip = selector.canSkipServerIsBusyBackoff() + s.True(skip) // Test for leader read. req = tikvrpc.NewRequest(tikvrpc.CmdGet, &kvrpcpb.GetRequest{Key: []byte("key")}) From d6a259adda98276c9415cf34f15618a65ce8f675 Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Tue, 27 Feb 2024 16:07:44 +0800 Subject: [PATCH 06/14] fix test Signed-off-by: crazycs520 --- internal/locate/replica_selector_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/locate/replica_selector_test.go b/internal/locate/replica_selector_test.go index c920031c2..f8fc5bc5e 100644 --- a/internal/locate/replica_selector_test.go +++ b/internal/locate/replica_selector_test.go @@ -298,8 +298,8 @@ func TestReplicaReadAccessPathByCase(t *testing.T) { "{addr: store3, replica-read: true, stale-read: false}"}, respErr: "", respRegionError: fakeEpochNotMatch, - backoffCnt: 2, - backoffDetail: []string{"tikvServerBusy+2"}, + backoffCnt: 1, + backoffDetail: []string{"tikvServerBusy+1"}, regionIsValid: true, }, } From d7ab10b8e417d11bab718e0be5601f03f8258cff Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Thu, 29 Feb 2024 10:21:24 +0800 Subject: [PATCH 07/14] add comment and refine code Signed-off-by: crazycs520 --- internal/locate/region_request.go | 19 +++++++++++++------ internal/locate/replica_selector_test.go | 12 ++++++------ 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/internal/locate/region_request.go b/internal/locate/region_request.go index 9db6c270f..6b4dacc2c 100644 --- a/internal/locate/region_request.go +++ b/internal/locate/region_request.go @@ -1248,9 +1248,9 @@ func (s *replicaSelector) updateLeader(leader *metapb.Peer) { func (s *replicaSelector) onServerIsBusy( bo *retry.Backoffer, ctx *RPCContext, req *tikvrpc.Request, serverIsBusy *errorpb.ServerIsBusy, ) (shouldRetry bool, err error) { - storeId := uint64(0) + var store *Store if ctx != nil && ctx.Store != nil { - storeId = ctx.Store.storeID + store = ctx.Store ctx.Store.updateServerLoadStats(serverIsBusy.EstimatedWaitMs) if serverIsBusy.EstimatedWaitMs != 0 { if s.busyThreshold != 0 { @@ -1270,7 +1270,7 @@ func (s *replicaSelector) onServerIsBusy( } backoffErr := errors.Errorf("server is busy, ctx: %v", ctx) if s.canFastRetry() { - s.addPendingBackoff(storeId, retry.BoTiKVServerBusy, backoffErr) + s.addPendingBackoff(store, retry.BoTiKVServerBusy, backoffErr) return true, nil } err = bo.Backoff(retry.BoTiKVServerBusy, backoffErr) @@ -1487,7 +1487,7 @@ func (s *RegionRequestSender) SendReqCtx( return nil, nil, retryTimes, err } if s.replicaSelector != nil { - if err := s.replicaSelector.backoffOnRetry(bo, rpcCtx.Store); err != nil { + if err := s.replicaSelector.backoffOnRetry(rpcCtx.Store, bo); err != nil { return nil, nil, retryTimes, err } } @@ -2454,14 +2454,20 @@ type backoffArgs struct { err error } -func (s *replicaSelector) addPendingBackoff(storeId uint64, cfg *retry.Config, err error) { +// addPendingBackoff adds pending backoff for the store. +func (s *replicaSelector) addPendingBackoff(store *Store, cfg *retry.Config, err error) { + storeId := uint64(0) + if store != nil { + storeId = store.storeID + } if s.pendingBackoffs == nil { s.pendingBackoffs = make(map[uint64]*backoffArgs) } s.pendingBackoffs[storeId] = &backoffArgs{cfg, err} } -func (s *replicaSelector) backoffOnRetry(bo *retry.Backoffer, store *Store) error { +// backoffOnRetry apply pending backoff on the store. +func (s *replicaSelector) backoffOnRetry(store *Store, bo *retry.Backoffer) error { storeId := uint64(0) if store != nil { storeId = store.storeID @@ -2474,6 +2480,7 @@ func (s *replicaSelector) backoffOnRetry(bo *retry.Backoffer, store *Store) erro return bo.Backoff(args.cfg, args.err) } +// backoffOnNoCandidate apply the largest base pending backoff when no candidate. func (s *replicaSelector) backoffOnNoCandidate(bo *retry.Backoffer) error { var args *backoffArgs // if there are multiple pending backoffs, choose the one with the largest base duration. diff --git a/internal/locate/replica_selector_test.go b/internal/locate/replica_selector_test.go index 101c8bf66..f4e9d6b5b 100644 --- a/internal/locate/replica_selector_test.go +++ b/internal/locate/replica_selector_test.go @@ -483,24 +483,24 @@ func TestPendingBackoff(t *testing.T) { req.EnableStaleWithMixedReplicaRead() selector, err := newReplicaSelector(s.cache, loc.Region, req) s.Nil(err) - selector.addPendingBackoff(0, retry.BoRegionScheduling, errors.New("err-0")) + selector.addPendingBackoff(nil, retry.BoRegionScheduling, errors.New("err-0")) s.Equal(1, len(selector.pendingBackoffs)) - selector.addPendingBackoff(1, retry.BoTiKVRPC, errors.New("err-1")) + selector.addPendingBackoff(&Store{storeID: 1}, retry.BoTiKVRPC, errors.New("err-1")) s.Equal(2, len(selector.pendingBackoffs)) - selector.addPendingBackoff(2, retry.BoTiKVDiskFull, errors.New("err-2")) + selector.addPendingBackoff(&Store{storeID: 2}, retry.BoTiKVDiskFull, errors.New("err-2")) s.Equal(3, len(selector.pendingBackoffs)) - selector.addPendingBackoff(1, retry.BoTiKVServerBusy, errors.New("err-3")) + selector.addPendingBackoff(&Store{storeID: 1}, retry.BoTiKVServerBusy, errors.New("err-3")) s.Equal(3, len(selector.pendingBackoffs)) bo := retry.NewNoopBackoff(context.Background()) _, ok := selector.pendingBackoffs[0] s.True(ok) - err = selector.backoffOnRetry(bo, nil) + err = selector.backoffOnRetry(nil, bo) s.NotNil(err) s.Equal("err-0", err.Error()) _, ok = selector.pendingBackoffs[0] s.False(ok) s.Equal(2, len(selector.pendingBackoffs)) - err = selector.backoffOnRetry(bo, &Store{storeID: 10}) + err = selector.backoffOnRetry(&Store{storeID: 10}, bo) s.Nil(err) s.Equal(2, len(selector.pendingBackoffs)) err = selector.backoffOnNoCandidate(bo) From 5c49640b7a5362a38a7002d4fd4ca803c405c7fc Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Thu, 29 Feb 2024 10:25:32 +0800 Subject: [PATCH 08/14] add comment and refine code Signed-off-by: crazycs520 --- internal/locate/region_request.go | 7 +++---- internal/locate/replica_selector_test.go | 8 +++++++- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/internal/locate/region_request.go b/internal/locate/region_request.go index 6b4dacc2c..8a20e95a9 100644 --- a/internal/locate/region_request.go +++ b/internal/locate/region_request.go @@ -2483,10 +2483,9 @@ func (s *replicaSelector) backoffOnRetry(store *Store, bo *retry.Backoffer) erro // backoffOnNoCandidate apply the largest base pending backoff when no candidate. func (s *replicaSelector) backoffOnNoCandidate(bo *retry.Backoffer) error { var args *backoffArgs - // if there are multiple pending backoffs, choose the one with the largest base duration. - for _, it := range s.pendingBackoffs { - if args == nil || args.cfg.Base() < it.cfg.Base() { - args = it + for _, pbo := range s.pendingBackoffs { + if args == nil || args.cfg.Base() < pbo.cfg.Base() { + args = pbo } } if args == nil { diff --git a/internal/locate/replica_selector_test.go b/internal/locate/replica_selector_test.go index f4e9d6b5b..b0fa6a8f5 100644 --- a/internal/locate/replica_selector_test.go +++ b/internal/locate/replica_selector_test.go @@ -483,6 +483,13 @@ func TestPendingBackoff(t *testing.T) { req.EnableStaleWithMixedReplicaRead() selector, err := newReplicaSelector(s.cache, loc.Region, req) s.Nil(err) + bo := retry.NewNoopBackoff(context.Background()) + err = selector.backoffOnRetry(nil, bo) + s.Nil(err) + err = selector.backoffOnRetry(&Store{storeID: 1}, bo) + s.Nil(err) + err = selector.backoffOnNoCandidate(bo) + s.Nil(err) selector.addPendingBackoff(nil, retry.BoRegionScheduling, errors.New("err-0")) s.Equal(1, len(selector.pendingBackoffs)) selector.addPendingBackoff(&Store{storeID: 1}, retry.BoTiKVRPC, errors.New("err-1")) @@ -491,7 +498,6 @@ func TestPendingBackoff(t *testing.T) { s.Equal(3, len(selector.pendingBackoffs)) selector.addPendingBackoff(&Store{storeID: 1}, retry.BoTiKVServerBusy, errors.New("err-3")) s.Equal(3, len(selector.pendingBackoffs)) - bo := retry.NewNoopBackoff(context.Background()) _, ok := selector.pendingBackoffs[0] s.True(ok) err = selector.backoffOnRetry(nil, bo) From f8a0b8e3dde2a5cbd01343856da6cb9a5dd352de Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Thu, 29 Feb 2024 10:29:42 +0800 Subject: [PATCH 09/14] fix lint Signed-off-by: crazycs520 --- internal/locate/replica_selector_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/locate/replica_selector_test.go b/internal/locate/replica_selector_test.go index b0fa6a8f5..6e8f6e9d0 100644 --- a/internal/locate/replica_selector_test.go +++ b/internal/locate/replica_selector_test.go @@ -3,7 +3,6 @@ package locate import ( "context" "fmt" - "github.com/pkg/errors" "math/rand" "sort" "strconv" @@ -16,6 +15,7 @@ import ( "github.com/pingcap/kvproto/pkg/errorpb" "github.com/pingcap/kvproto/pkg/kvrpcpb" "github.com/pingcap/kvproto/pkg/metapb" + "github.com/pkg/errors" "github.com/stretchr/testify/suite" "github.com/tikv/client-go/v2/config/retry" "github.com/tikv/client-go/v2/internal/apicodec" From 6a3ae20671c28e99378a6e8e08ac54c010869862 Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Thu, 29 Feb 2024 10:38:25 +0800 Subject: [PATCH 10/14] fix test Signed-off-by: crazycs520 --- internal/locate/replica_selector_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/locate/replica_selector_test.go b/internal/locate/replica_selector_test.go index fb0f18bf4..8132fa25a 100644 --- a/internal/locate/replica_selector_test.go +++ b/internal/locate/replica_selector_test.go @@ -321,8 +321,8 @@ func TestReplicaReadAccessPathByCase(t *testing.T) { "{addr: store3, replica-read: true, stale-read: false}"}, respErr: "", respRegionError: fakeEpochNotMatch, - backoffCnt: 2, - backoffDetail: []string{"tikvServerBusy+2"}, + backoffCnt: 1, + backoffDetail: []string{"tikvServerBusy+1"}, regionIsValid: true, }, } From eb0f14d619d68adbed141bb378839dc914d0f269 Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Thu, 29 Feb 2024 17:01:11 +0800 Subject: [PATCH 11/14] add more comment Signed-off-by: crazycs520 --- internal/locate/region_request.go | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/internal/locate/region_request.go b/internal/locate/region_request.go index 5806acb83..9942b5cc8 100644 --- a/internal/locate/region_request.go +++ b/internal/locate/region_request.go @@ -285,7 +285,22 @@ type replicaSelector struct { // TiKV can reject the request when its estimated wait duration exceeds busyThreshold. // Then, the client will receive a ServerIsBusy error and choose another replica to retry. busyThreshold time.Duration - // pendingBackoffs records the pending backoff for fast retry. + // pendingBackoffs records the pending backoff by store_id for fast retry. Here are some examples to show how it works: + // Example-1, fast retry and success: + // 1. send req to store 1, got ServerIsBusy region error, record `store1 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. + // 2. retry in store2, and success. + // Since the request is success, we can skip the backoff and fast return result to user. + // Example-2: fast retry different replicas but all failed: + // 1. send req to store 1, got ServerIsBusy region error, record `store1 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. + // 2. send req to store 2, got ServerIsBusy region error, record `store2 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. + // 3. send req to store 3, got ServerIsBusy region error, record `store3 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. + // 4. no candidate since all stores are busy. But before return no candidate error to up layer, we need to call backoffOnNoCandidate function + // to apply a max pending backoff, the backoff is to avoid frequent access and increase the pressure on the cluster. + // Example-3: fast retry same replica: + // 1. send req to store 1, got ServerIsBusy region error, record `store1 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. + // 2. assume store 2 and store 3 are unreachable. + // 4. re-send req to store 1 with replica-read. But before re-send to store1, we need to call backoffOnRetry function + // to apply pending BoTiKVServerBusy backoff, the backoff is to avoid frequent access and increase the pressure on the cluster. pendingBackoffs map[uint64]*backoffArgs } @@ -2476,7 +2491,7 @@ func (s *replicaSelector) addPendingBackoff(store *Store, cfg *retry.Config, err s.pendingBackoffs[storeId] = &backoffArgs{cfg, err} } -// backoffOnRetry apply pending backoff on the store. +// backoffOnRetry apply pending backoff on the store when retry in this store. func (s *replicaSelector) backoffOnRetry(store *Store, bo *retry.Backoffer) error { storeId := uint64(0) if store != nil { From 5c82a08714506e506a8c80daceafeba75e404686 Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Thu, 29 Feb 2024 22:49:50 +0800 Subject: [PATCH 12/14] address comment Signed-off-by: crazycs520 --- internal/locate/region_request.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/locate/region_request.go b/internal/locate/region_request.go index 9942b5cc8..847b61d08 100644 --- a/internal/locate/region_request.go +++ b/internal/locate/region_request.go @@ -287,19 +287,19 @@ type replicaSelector struct { busyThreshold time.Duration // pendingBackoffs records the pending backoff by store_id for fast retry. Here are some examples to show how it works: // Example-1, fast retry and success: - // 1. send req to store 1, got ServerIsBusy region error, record `store1 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. - // 2. retry in store2, and success. - // Since the request is success, we can skip the backoff and fast return result to user. + // 1. send req to store 1, got ServerIsBusy region error, record `store1 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. + // 2. retry in store2, and success. + // Since the request is success, we can skip the backoff and fast return result to user. // Example-2: fast retry different replicas but all failed: - // 1. send req to store 1, got ServerIsBusy region error, record `store1 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. - // 2. send req to store 2, got ServerIsBusy region error, record `store2 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. - // 3. send req to store 3, got ServerIsBusy region error, record `store3 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. + // 1. send req to store 1, got ServerIsBusy region error, record `store1 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. + // 2. send req to store 2, got ServerIsBusy region error, record `store2 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. + // 3. send req to store 3, got ServerIsBusy region error, record `store3 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. // 4. no candidate since all stores are busy. But before return no candidate error to up layer, we need to call backoffOnNoCandidate function // to apply a max pending backoff, the backoff is to avoid frequent access and increase the pressure on the cluster. // Example-3: fast retry same replica: - // 1. send req to store 1, got ServerIsBusy region error, record `store1 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. + // 1. send req to store 1, got ServerIsBusy region error, record `store1 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. // 2. assume store 2 and store 3 are unreachable. - // 4. re-send req to store 1 with replica-read. But before re-send to store1, we need to call backoffOnRetry function + // 3. re-send req to store 1 with replica-read. But before re-send to store1, we need to call backoffOnRetry function // to apply pending BoTiKVServerBusy backoff, the backoff is to avoid frequent access and increase the pressure on the cluster. pendingBackoffs map[uint64]*backoffArgs } From 589c2d8b4d3c977535039a13ee4d882720157320 Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Thu, 29 Feb 2024 22:54:05 +0800 Subject: [PATCH 13/14] refine comment Signed-off-by: crazycs520 --- internal/locate/region_request.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/locate/region_request.go b/internal/locate/region_request.go index 847b61d08..7ed9b7234 100644 --- a/internal/locate/region_request.go +++ b/internal/locate/region_request.go @@ -287,17 +287,17 @@ type replicaSelector struct { busyThreshold time.Duration // pendingBackoffs records the pending backoff by store_id for fast retry. Here are some examples to show how it works: // Example-1, fast retry and success: - // 1. send req to store 1, got ServerIsBusy region error, record `store1 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. - // 2. retry in store2, and success. + // 1. send req to store 1, got ServerIsBusy region error, record `store1 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. + // 2. retry in store2, and success. // Since the request is success, we can skip the backoff and fast return result to user. // Example-2: fast retry different replicas but all failed: - // 1. send req to store 1, got ServerIsBusy region error, record `store1 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. + // 1. send req to store 1, got ServerIsBusy region error, record `store1 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. // 2. send req to store 2, got ServerIsBusy region error, record `store2 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. // 3. send req to store 3, got ServerIsBusy region error, record `store3 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. // 4. no candidate since all stores are busy. But before return no candidate error to up layer, we need to call backoffOnNoCandidate function // to apply a max pending backoff, the backoff is to avoid frequent access and increase the pressure on the cluster. // Example-3: fast retry same replica: - // 1. send req to store 1, got ServerIsBusy region error, record `store1 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. + // 1. send req to store 1, got ServerIsBusy region error, record `store1 -> BoTiKVServerBusy` backoff in pendingBackoffs and fast retry next replica. // 2. assume store 2 and store 3 are unreachable. // 3. re-send req to store 1 with replica-read. But before re-send to store1, we need to call backoffOnRetry function // to apply pending BoTiKVServerBusy backoff, the backoff is to avoid frequent access and increase the pressure on the cluster. From a156b79f8aa20c7dd526fc0b6b37d6388d2d3445 Mon Sep 17 00:00:00 2001 From: crazycs520 Date: Mon, 4 Mar 2024 10:54:02 +0800 Subject: [PATCH 14/14] address comment Signed-off-by: crazycs520 --- internal/locate/region_cache.go | 17 ++++++----------- internal/locate/region_request.go | 5 ++++- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/internal/locate/region_cache.go b/internal/locate/region_cache.go index 3018f05d2..f2e574876 100644 --- a/internal/locate/region_cache.go +++ b/internal/locate/region_cache.go @@ -3253,18 +3253,13 @@ func (s *Store) GetPeerAddr() string { } func (s *Store) updateServerLoadStats(estimatedWaitMs uint32) { - if estimatedWaitMs != 0 { - estimatedWait := time.Duration(estimatedWaitMs) * time.Millisecond - // Update the estimated wait time of the store. - loadStats := &storeLoadStats{ - estimatedWait: estimatedWait, - waitTimeUpdatedAt: time.Now(), - } - s.loadStats.Store(loadStats) - } else { - // Mark the server is busy (the next incoming READs could be redirect to expected followers.) - s.healthStatus.markAlreadySlow() + estimatedWait := time.Duration(estimatedWaitMs) * time.Millisecond + // Update the estimated wait time of the store. + loadStats := &storeLoadStats{ + estimatedWait: estimatedWait, + waitTimeUpdatedAt: time.Now(), } + s.loadStats.Store(loadStats) } // EstimatedWaitTime returns an optimistic estimation of how long a request will wait in the store. diff --git a/internal/locate/region_request.go b/internal/locate/region_request.go index 7ed9b7234..f05f69dc7 100644 --- a/internal/locate/region_request.go +++ b/internal/locate/region_request.go @@ -1276,8 +1276,8 @@ func (s *replicaSelector) onServerIsBusy( var store *Store if ctx != nil && ctx.Store != nil { store = ctx.Store - ctx.Store.updateServerLoadStats(serverIsBusy.EstimatedWaitMs) if serverIsBusy.EstimatedWaitMs != 0 { + ctx.Store.updateServerLoadStats(serverIsBusy.EstimatedWaitMs) if s.busyThreshold != 0 { // do not retry with batched coprocessor requests. // it'll be region misses if we send the tasks to replica. @@ -1291,6 +1291,9 @@ func (s *replicaSelector) onServerIsBusy( s.state = &tryIdleReplica{leaderIdx: state.leaderIdx} } } + } else { + // Mark the server is busy (the next incoming READs could be redirect to expected followers.) + ctx.Store.healthStatus.markAlreadySlow() } } backoffErr := errors.Errorf("server is busy, ctx: %v", ctx)