From f3a714ac8f3d30940d7a78c606bccd19ea606e9b Mon Sep 17 00:00:00 2001 From: kennytm Date: Tue, 19 May 2020 00:56:53 +0800 Subject: [PATCH 1/2] utils: return a multierr in WithRetry rather than just the last error --- pkg/restore/backoff_test.go | 39 +++++++++++++++++++++++++++++++++++-- pkg/restore/import.go | 11 ++++++++--- pkg/utils/retry.go | 15 +++++++++----- 3 files changed, 55 insertions(+), 10 deletions(-) diff --git a/pkg/restore/backoff_test.go b/pkg/restore/backoff_test.go index 5ee63e885..db576e67d 100644 --- a/pkg/restore/backoff_test.go +++ b/pkg/restore/backoff_test.go @@ -8,6 +8,7 @@ import ( . "github.com/pingcap/check" "github.com/pingcap/tidb/util/testleak" + "go.uber.org/multierr" "github.com/pingcap/br/pkg/mock" "github.com/pingcap/br/pkg/restore" @@ -30,6 +31,25 @@ func (s *testBackofferSuite) TearDownSuite(c *C) { testleak.AfterTest(c)() } +func (s *testBackofferSuite) TestBackoffWithSuccess(c *C) { + var counter int + backoffer := restore.NewBackoffer(10, time.Nanosecond, time.Nanosecond) + err := utils.WithRetry(context.Background(), func() error { + defer func() { counter++ }() + switch counter { + case 0: + return restore.ErrGRPC + case 1: + return restore.ErrEpochNotMatch + case 2: + return nil + } + return nil + }, backoffer) + c.Assert(counter, Equals, 3) + c.Assert(err, IsNil) +} + func (s *testBackofferSuite) TestBackoffWithFatalError(c *C) { var counter int backoffer := restore.NewBackoffer(10, time.Nanosecond, time.Nanosecond) @@ -46,7 +66,11 @@ func (s *testBackofferSuite) TestBackoffWithFatalError(c *C) { return nil }, backoffer) c.Assert(counter, Equals, 3) - c.Assert(err, Equals, restore.ErrRangeIsEmpty) + c.Assert(multierr.Errors(err), DeepEquals, []error{ + restore.ErrGRPC, + restore.ErrEpochNotMatch, + restore.ErrRangeIsEmpty, + }) } func (s *testBackofferSuite) TestBackoffWithRetryableError(c *C) { @@ -57,5 +81,16 @@ func (s *testBackofferSuite) TestBackoffWithRetryableError(c *C) { return restore.ErrEpochNotMatch }, backoffer) c.Assert(counter, Equals, 10) - c.Assert(err, Equals, restore.ErrEpochNotMatch) + c.Assert(multierr.Errors(err), DeepEquals, []error{ + restore.ErrEpochNotMatch, + restore.ErrEpochNotMatch, + restore.ErrEpochNotMatch, + restore.ErrEpochNotMatch, + restore.ErrEpochNotMatch, + restore.ErrEpochNotMatch, + restore.ErrEpochNotMatch, + restore.ErrEpochNotMatch, + restore.ErrEpochNotMatch, + restore.ErrEpochNotMatch, + }) } diff --git a/pkg/restore/import.go b/pkg/restore/import.go index 2f65b4ccf..5d91bbb6e 100644 --- a/pkg/restore/import.go +++ b/pkg/restore/import.go @@ -16,6 +16,7 @@ import ( "github.com/pingcap/kvproto/pkg/kvrpcpb" "github.com/pingcap/log" "github.com/pingcap/pd/v4/pkg/codec" + "go.uber.org/multierr" "go.uber.org/zap" "google.golang.org/grpc" "google.golang.org/grpc/credentials" @@ -228,6 +229,7 @@ func (importer *FileImporter) Import( log.Debug("scan regions", zap.Stringer("file", file), zap.Int("count", len(regionInfos))) // Try to download and ingest the file in every region + regionLoop: for _, regionInfo := range regionInfos { info := regionInfo // Try to download file. @@ -242,9 +244,12 @@ func (importer *FileImporter) Import( return e }, newDownloadSSTBackoffer()) if errDownload != nil { - if errDownload == ErrRewriteRuleNotFound || errDownload == ErrRangeIsEmpty { - // Skip this region - continue + for _, e := range multierr.Errors(errDownload) { + switch e { + case ErrRewriteRuleNotFound, ErrRangeIsEmpty: + // Skip this region + continue regionLoop + } } log.Error("download file failed", zap.Stringer("file", file), diff --git a/pkg/utils/retry.go b/pkg/utils/retry.go index c1b8008e9..07a84de62 100644 --- a/pkg/utils/retry.go +++ b/pkg/utils/retry.go @@ -5,6 +5,8 @@ package utils import ( "context" "time" + + "go.uber.org/multierr" ) // RetryableFunc presents a retryable operation. @@ -18,25 +20,28 @@ type Backoffer interface { Attempt() int } -// WithRetry retrys a given operation with a backoff policy. +// WithRetry retries a given operation with a backoff policy. +// +// Returns nil if `retryableFunc` succeeded at least once. Otherwise, returns a +// multierr containing all errors encountered. func WithRetry( ctx context.Context, retryableFunc RetryableFunc, backoffer Backoffer, ) error { - var lastErr error + var allErrors error for backoffer.Attempt() > 0 { err := retryableFunc() if err != nil { - lastErr = err + allErrors = multierr.Append(allErrors, err) select { case <-ctx.Done(): - return lastErr + return allErrors case <-time.After(backoffer.NextBackoff(err)): } } else { return nil } } - return lastErr + return allErrors } From 6f65ac46a19388afde07c572bb20b656a75a84d7 Mon Sep 17 00:00:00 2001 From: kennytm Date: Tue, 19 May 2020 01:07:13 +0800 Subject: [PATCH 2/2] restore: treat ErrDownloadFailed as retryable --- pkg/restore/backoff.go | 5 +++-- pkg/restore/backoff_test.go | 5 ++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/pkg/restore/backoff.go b/pkg/restore/backoff.go index 77b57b3e7..6fa31e442 100644 --- a/pkg/restore/backoff.go +++ b/pkg/restore/backoff.go @@ -27,7 +27,8 @@ var ( ErrRangeIsEmpty = errors.NewNoStackError("range is empty") // ErrGRPC indicates any gRPC communication error. This error can be retried. ErrGRPC = errors.NewNoStackError("gRPC error") - // ErrDownloadFailed indicates a generic, non-retryable download error. + // ErrDownloadFailed indicates a generic download error, expected to be + // retryable. ErrDownloadFailed = errors.NewNoStackError("download sst failed") // ErrIngestFailed indicates a generic, retryable ingest error. ErrIngestFailed = errors.NewNoStackError("ingest sst failed") @@ -72,7 +73,7 @@ func newDownloadSSTBackoffer() utils.Backoffer { func (bo *importerBackoffer) NextBackoff(err error) time.Duration { switch errors.Cause(err) { - case ErrGRPC, ErrEpochNotMatch, ErrIngestFailed: + case ErrGRPC, ErrEpochNotMatch, ErrDownloadFailed, ErrIngestFailed: bo.delayTime = 2 * bo.delayTime bo.attempt-- case ErrRangeIsEmpty, ErrRewriteRuleNotFound: diff --git a/pkg/restore/backoff_test.go b/pkg/restore/backoff_test.go index db576e67d..cb2b9a19d 100644 --- a/pkg/restore/backoff_test.go +++ b/pkg/restore/backoff_test.go @@ -61,14 +61,17 @@ func (s *testBackofferSuite) TestBackoffWithFatalError(c *C) { case 1: return restore.ErrEpochNotMatch case 2: + return restore.ErrDownloadFailed + case 3: return restore.ErrRangeIsEmpty } return nil }, backoffer) - c.Assert(counter, Equals, 3) + c.Assert(counter, Equals, 4) c.Assert(multierr.Errors(err), DeepEquals, []error{ restore.ErrGRPC, restore.ErrEpochNotMatch, + restore.ErrDownloadFailed, restore.ErrRangeIsEmpty, }) }