From 7daf7b3c1e6d7d376a8f8682a2ad85cb7666393c Mon Sep 17 00:00:00 2001 From: Chunzhu Li Date: Wed, 25 Nov 2020 12:17:06 +0800 Subject: [PATCH 1/3] retry for read index not ready and proposal in merging mode --- go.mod1 | 2 +- go.sum1 | 2 ++ pkg/backup/client.go | 4 +++- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/go.mod1 b/go.mod1 index 593397e7d..5fb855de5 100644 --- a/go.mod1 +++ b/go.mod1 @@ -16,7 +16,7 @@ require ( github.com/pingcap/check v0.0.0-20200212061837-5e12011dc712 github.com/pingcap/errors v0.11.5-0.20201029093017-5a7df2af2ac7 github.com/pingcap/failpoint v0.0.0-20200702092429-9f69995143ce - github.com/pingcap/kvproto v0.0.0-20201113092725-08f2872278eb + github.com/pingcap/kvproto v0.0.0-20201124110645-494a2fb764b7 github.com/pingcap/log v0.0.0-20201112100606-8f1e84a3abc8 github.com/pingcap/parser v0.0.0-20201112105242-773b8b74f44e github.com/pingcap/tidb v1.1.0-beta.0.20201119091753-38bbb0dd2197 diff --git a/go.sum1 b/go.sum1 index 6a39b8637..b6631c4d2 100644 --- a/go.sum1 +++ b/go.sum1 @@ -755,6 +755,8 @@ github.com/pingcap/kvproto v0.0.0-20200420075417-e0c6e8842f22/go.mod h1:IOdRDPLy github.com/pingcap/kvproto v0.0.0-20200810113304-6157337686b1/go.mod h1:IOdRDPLyda8GX2hE/jO7gqaCV/PNFh8BZQCQZXfIOqI= github.com/pingcap/kvproto v0.0.0-20201113092725-08f2872278eb h1:K3r4KjVQeD4nLnfj44ibdLIXnUh58aQpkgVNWuBO9z0= github.com/pingcap/kvproto v0.0.0-20201113092725-08f2872278eb/go.mod h1:IOdRDPLyda8GX2hE/jO7gqaCV/PNFh8BZQCQZXfIOqI= +github.com/pingcap/kvproto v0.0.0-20201124110645-494a2fb764b7 h1:xi36VGntRELtpBBAUkvx2XrCxSLwrOVEnbLnGT/Kx5g= +github.com/pingcap/kvproto v0.0.0-20201124110645-494a2fb764b7/go.mod h1:IOdRDPLyda8GX2hE/jO7gqaCV/PNFh8BZQCQZXfIOqI= github.com/pingcap/log v0.0.0-20191012051959-b742a5d432e9 h1:AJD9pZYm72vMgPcQDww9rkZ1DnWfl0pXV3BOWlkYIjA= github.com/pingcap/log v0.0.0-20191012051959-b742a5d432e9/go.mod h1:4rbK1p9ILyIfb6hU7OG2CiWSqMXnp3JMbiaVJ6mvoY8= github.com/pingcap/log v0.0.0-20200117041106-d28c14d3b1cd h1:CV3VsP3Z02MVtdpTMfEgRJ4T9NGgGTxdHpJerent7rM= diff --git a/pkg/backup/client.go b/pkg/backup/client.go index 47bb71c61..69b309a1a 100644 --- a/pkg/backup/client.go +++ b/pkg/backup/client.go @@ -709,7 +709,9 @@ func onBackupResponse( regionErr.RegionNotFound != nil || regionErr.ServerIsBusy != nil || regionErr.StaleCommand != nil || - regionErr.StoreNotMatch != nil) { + regionErr.StoreNotMatch != nil || + regionErr.ReadIndexNotReady != nil || + regionErr.ProposalInMergingMode != nil) { log.Error("unexpect region error", zap.Reflect("RegionError", regionErr)) return nil, backoffMs, errors.Annotatef(berrors.ErrKVUnknown, "storeID: %d onBackupResponse error %v", storeID, v) } From ce5cce9e395491911687a8c351ab8573c0f1d318 Mon Sep 17 00:00:00 2001 From: Chunzhu Li Date: Fri, 27 Nov 2020 14:37:34 +0800 Subject: [PATCH 2/3] add unit test --- errors.toml | 2 +- pkg/backup/client.go | 11 ++++++----- pkg/backup/client_test.go | 41 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 6 deletions(-) diff --git a/errors.toml b/errors.toml index 79ac8ce67..ea431923d 100644 --- a/errors.toml +++ b/errors.toml @@ -1,5 +1,5 @@ # AUTOGENERATED BY github.com/pingcap/tiup/components/errdoc/errdoc-gen -# DO NOT EDIT THIS FILE, PLEASE CHANGE ERROR DEFINITION IF CONTENT IMPROPER. +# YOU CAN CHANGE THE 'description'/'workaround' FIELDS IF THEM ARE IMPROPER. ["BR:Backup:ErrBackupChecksumMismatch"] error = ''' diff --git a/pkg/backup/client.go b/pkg/backup/client.go index 24e3c0340..776711001 100644 --- a/pkg/backup/client.go +++ b/pkg/backup/client.go @@ -687,14 +687,15 @@ func (bc *Client) fineGrainedBackup( } } -func onBackupResponse( +// OnBackupResponse checks the backup resp, decides whether to retry and generate the error. +func OnBackupResponse( storeID uint64, bo *tikv.Backoffer, backupTS uint64, lockResolver *tikv.LockResolver, resp *kvproto.BackupResponse, ) (*kvproto.BackupResponse, int, error) { - log.Debug("onBackupResponse", zap.Reflect("resp", resp)) + log.Debug("OnBackupResponse", zap.Reflect("resp", resp)) if resp.Error == nil { return resp, 0, nil } @@ -716,7 +717,7 @@ func onBackupResponse( } // Backup should not meet error other than KeyLocked. log.Error("unexpect kv error", zap.Reflect("KvError", v.KvError)) - return nil, backoffMs, errors.Annotatef(berrors.ErrKVUnknown, "storeID: %d onBackupResponse error %v", storeID, v) + return nil, backoffMs, errors.Annotatef(berrors.ErrKVUnknown, "storeID: %d OnBackupResponse error %v", storeID, v) case *kvproto.Error_RegionError: regionErr := v.RegionError @@ -730,7 +731,7 @@ func onBackupResponse( regionErr.ReadIndexNotReady != nil || regionErr.ProposalInMergingMode != nil) { log.Error("unexpect region error", zap.Reflect("RegionError", regionErr)) - return nil, backoffMs, errors.Annotatef(berrors.ErrKVUnknown, "storeID: %d onBackupResponse error %v", storeID, v) + return nil, backoffMs, errors.Annotatef(berrors.ErrKVUnknown, "storeID: %d OnBackupResponse error %v", storeID, v) } log.Warn("backup occur region error", zap.Reflect("RegionError", regionErr), @@ -789,7 +790,7 @@ func (bc *Client) handleFineGrained( // Handle responses with the same backoffer. func(resp *kvproto.BackupResponse) error { response, backoffMs, err1 := - onBackupResponse(storeID, bo, backupTS, lockResolver, resp) + OnBackupResponse(storeID, bo, backupTS, lockResolver, resp) if err1 != nil { return err1 } diff --git a/pkg/backup/client_test.go b/pkg/backup/client_test.go index 8b6931c37..0dd1d19ab 100644 --- a/pkg/backup/client_test.go +++ b/pkg/backup/client_test.go @@ -4,14 +4,17 @@ package backup_test import ( "context" + "github.com/pingcap/kvproto/pkg/errorpb" "math" "testing" "time" . "github.com/pingcap/check" + kvproto "github.com/pingcap/kvproto/pkg/backup" "github.com/pingcap/parser/model" "github.com/pingcap/tidb/kv" "github.com/pingcap/tidb/store/mockstore/mocktikv" + "github.com/pingcap/tidb/store/tikv" "github.com/pingcap/tidb/store/tikv/oracle" "github.com/pingcap/tidb/tablecodec" "github.com/pingcap/tidb/util/codec" @@ -140,3 +143,41 @@ func (r *testBackup) TestBuildTableRange(c *C) { {StartKey: tablecodec.EncodeRowKey(7, low), EndKey: tablecodec.EncodeRowKey(7, high)}, }) } + +func (r *testBackup) TestOnBackupRegionErrorResponse(c *C) { + type Case struct { + storeID uint64 + bo *tikv.Backoffer + backupTS uint64 + lockResolver *tikv.LockResolver + resp *kvproto.BackupResponse + exceptedBackoffMs int + exceptedErr bool + } + newBackupRegionErrorResp := func(regionError *errorpb.Error) *kvproto.BackupResponse { + return &kvproto.BackupResponse{Error: &kvproto.Error{Detail: &kvproto.Error_RegionError{RegionError: regionError}}} + } + + cases := []Case{ + {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{NotLeader:&errorpb.NotLeader{}}), exceptedBackoffMs: 1000, exceptedErr: false}, + {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{RegionNotFound:&errorpb.RegionNotFound{}}), exceptedBackoffMs: 1000, exceptedErr: false}, + {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{KeyNotInRegion:&errorpb.KeyNotInRegion{}}), exceptedBackoffMs: 0, exceptedErr: true}, + {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{EpochNotMatch:&errorpb.EpochNotMatch{}}), exceptedBackoffMs: 1000, exceptedErr: false}, + {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{ServerIsBusy:&errorpb.ServerIsBusy{}}), exceptedBackoffMs: 1000, exceptedErr: false}, + {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{StaleCommand:&errorpb.StaleCommand{}}), exceptedBackoffMs: 1000, exceptedErr: false}, + {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{StoreNotMatch:&errorpb.StoreNotMatch{}}), exceptedBackoffMs: 1000, exceptedErr: false}, + {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{RaftEntryTooLarge:&errorpb.RaftEntryTooLarge{}}), exceptedBackoffMs: 0, exceptedErr: true}, + {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{ReadIndexNotReady:&errorpb.ReadIndexNotReady{}}), exceptedBackoffMs: 1000, exceptedErr: false}, + {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{ProposalInMergingMode:&errorpb.ProposalInMergingMode{}}), exceptedBackoffMs: 1000, exceptedErr: false}, + } + for _, cs := range cases { + c.Log(cs) + _, backoffMs, err := backup.OnBackupResponse(cs.storeID, cs.bo, cs.backupTS, cs.lockResolver, cs.resp) + c.Assert(backoffMs, Equals, cs.exceptedBackoffMs) + if cs.exceptedErr { + c.Assert(err, NotNil) + } else { + c.Assert(err, IsNil) + } + } +} From f0017182e9dad474e15688d001d9fd262335d54e Mon Sep 17 00:00:00 2001 From: Chunzhu Li Date: Fri, 27 Nov 2020 14:45:52 +0800 Subject: [PATCH 3/3] fix fmt --- pkg/backup/client_test.go | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/pkg/backup/client_test.go b/pkg/backup/client_test.go index 0dd1d19ab..065de9e59 100644 --- a/pkg/backup/client_test.go +++ b/pkg/backup/client_test.go @@ -4,13 +4,13 @@ package backup_test import ( "context" - "github.com/pingcap/kvproto/pkg/errorpb" "math" "testing" "time" . "github.com/pingcap/check" kvproto "github.com/pingcap/kvproto/pkg/backup" + "github.com/pingcap/kvproto/pkg/errorpb" "github.com/pingcap/parser/model" "github.com/pingcap/tidb/kv" "github.com/pingcap/tidb/store/mockstore/mocktikv" @@ -146,29 +146,29 @@ func (r *testBackup) TestBuildTableRange(c *C) { func (r *testBackup) TestOnBackupRegionErrorResponse(c *C) { type Case struct { - storeID uint64 - bo *tikv.Backoffer - backupTS uint64 - lockResolver *tikv.LockResolver - resp *kvproto.BackupResponse + storeID uint64 + bo *tikv.Backoffer + backupTS uint64 + lockResolver *tikv.LockResolver + resp *kvproto.BackupResponse exceptedBackoffMs int - exceptedErr bool + exceptedErr bool } newBackupRegionErrorResp := func(regionError *errorpb.Error) *kvproto.BackupResponse { return &kvproto.BackupResponse{Error: &kvproto.Error{Detail: &kvproto.Error_RegionError{RegionError: regionError}}} } cases := []Case{ - {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{NotLeader:&errorpb.NotLeader{}}), exceptedBackoffMs: 1000, exceptedErr: false}, - {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{RegionNotFound:&errorpb.RegionNotFound{}}), exceptedBackoffMs: 1000, exceptedErr: false}, - {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{KeyNotInRegion:&errorpb.KeyNotInRegion{}}), exceptedBackoffMs: 0, exceptedErr: true}, - {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{EpochNotMatch:&errorpb.EpochNotMatch{}}), exceptedBackoffMs: 1000, exceptedErr: false}, - {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{ServerIsBusy:&errorpb.ServerIsBusy{}}), exceptedBackoffMs: 1000, exceptedErr: false}, - {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{StaleCommand:&errorpb.StaleCommand{}}), exceptedBackoffMs: 1000, exceptedErr: false}, - {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{StoreNotMatch:&errorpb.StoreNotMatch{}}), exceptedBackoffMs: 1000, exceptedErr: false}, - {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{RaftEntryTooLarge:&errorpb.RaftEntryTooLarge{}}), exceptedBackoffMs: 0, exceptedErr: true}, - {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{ReadIndexNotReady:&errorpb.ReadIndexNotReady{}}), exceptedBackoffMs: 1000, exceptedErr: false}, - {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{ProposalInMergingMode:&errorpb.ProposalInMergingMode{}}), exceptedBackoffMs: 1000, exceptedErr: false}, + {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{NotLeader: &errorpb.NotLeader{}}), exceptedBackoffMs: 1000, exceptedErr: false}, + {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{RegionNotFound: &errorpb.RegionNotFound{}}), exceptedBackoffMs: 1000, exceptedErr: false}, + {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{KeyNotInRegion: &errorpb.KeyNotInRegion{}}), exceptedBackoffMs: 0, exceptedErr: true}, + {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{EpochNotMatch: &errorpb.EpochNotMatch{}}), exceptedBackoffMs: 1000, exceptedErr: false}, + {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{ServerIsBusy: &errorpb.ServerIsBusy{}}), exceptedBackoffMs: 1000, exceptedErr: false}, + {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{StaleCommand: &errorpb.StaleCommand{}}), exceptedBackoffMs: 1000, exceptedErr: false}, + {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{StoreNotMatch: &errorpb.StoreNotMatch{}}), exceptedBackoffMs: 1000, exceptedErr: false}, + {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{RaftEntryTooLarge: &errorpb.RaftEntryTooLarge{}}), exceptedBackoffMs: 0, exceptedErr: true}, + {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{ReadIndexNotReady: &errorpb.ReadIndexNotReady{}}), exceptedBackoffMs: 1000, exceptedErr: false}, + {storeID: 1, backupTS: 421123291611137, resp: newBackupRegionErrorResp(&errorpb.Error{ProposalInMergingMode: &errorpb.ProposalInMergingMode{}}), exceptedBackoffMs: 1000, exceptedErr: false}, } for _, cs := range cases { c.Log(cs)