From b3f965c7d49cd48b7c35c33d373021b4b7e77ad7 Mon Sep 17 00:00:00 2001
From: godchen <qingxiang.chen@zilliz.com>
Date: Sat, 18 Sep 2021 10:49:51 +0800
Subject: [PATCH] Add unrecoverable error for retry. (#7828)

Signed-off-by: godchen <qingxiang.chen@zilliz.com>
---
 internal/rootcoord/root_coord_test.go |  6 +++---
 internal/rootcoord/suffix_snapshot.go |  2 +-
 internal/util/retry/options.go        |  2 +-
 internal/util/retry/retry.go          | 26 +++++++++++++++++---------
 internal/util/retry/retry_test.go     | 14 ++++++++++++++
 5 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/internal/rootcoord/root_coord_test.go b/internal/rootcoord/root_coord_test.go
index 47175cba84..4faf39b964 100644
--- a/internal/rootcoord/root_coord_test.go
+++ b/internal/rootcoord/root_coord_test.go
@@ -406,7 +406,7 @@ type loadPrefixFailKV struct {
 
 // LoadWithPrefix override behavior
 func (kv *loadPrefixFailKV) LoadWithPrefix(key string) ([]string, []string, error) {
-	return []string{}, []string{}, retry.NoRetryError(errors.New("mocked fail"))
+	return []string{}, []string{}, retry.Unrecoverable(errors.New("mocked fail"))
 }
 
 func TestRootCoordInit(t *testing.T) {
@@ -441,7 +441,7 @@ func TestRootCoordInit(t *testing.T) {
 	err = core.Register()
 	assert.Nil(t, err)
 	core.kvBaseCreate = func(string) (kv.TxnKV, error) {
-		return nil, retry.NoRetryError(errors.New("injected"))
+		return nil, retry.Unrecoverable(errors.New("injected"))
 	}
 	err = core.Init()
 	assert.NotNil(t, err)
@@ -459,7 +459,7 @@ func TestRootCoordInit(t *testing.T) {
 	assert.Nil(t, err)
 	core.kvBaseCreate = func(root string) (kv.TxnKV, error) {
 		if root == Params.MetaRootPath {
-			return nil, retry.NoRetryError(errors.New("injected"))
+			return nil, retry.Unrecoverable(errors.New("injected"))
 		}
 		return memkv.NewMemoryKV(), nil
 	}
diff --git a/internal/rootcoord/suffix_snapshot.go b/internal/rootcoord/suffix_snapshot.go
index 65659a45b1..b1d210c10a 100644
--- a/internal/rootcoord/suffix_snapshot.go
+++ b/internal/rootcoord/suffix_snapshot.go
@@ -76,7 +76,7 @@ var _ kv.SnapShotKV = (*suffixSnapshot)(nil)
 // newSuffixSnapshot creates a newSuffixSnapshot with provided kv
 func newSuffixSnapshot(txnKV kv.TxnKV, sep, root, snapshot string) (*suffixSnapshot, error) {
 	if txnKV == nil {
-		return nil, retry.NoRetryError(errors.New("txnKV is nil"))
+		return nil, retry.Unrecoverable(errors.New("txnKV is nil"))
 	}
 
 	// handles trailing / logic
diff --git a/internal/util/retry/options.go b/internal/util/retry/options.go
index 52073d93e4..ebfa5139e6 100644
--- a/internal/util/retry/options.go
+++ b/internal/util/retry/options.go
@@ -19,7 +19,7 @@ type Config struct {
 	maxSleepTime time.Duration
 }
 
-func NewDefaultConfig() *Config {
+func newDefaultConfig() *Config {
 	return &Config{
 		attempts:     uint(10),
 		sleep:        200 * time.Millisecond,
diff --git a/internal/util/retry/retry.go b/internal/util/retry/retry.go
index ad5fb31c1a..555d9cce42 100644
--- a/internal/util/retry/retry.go
+++ b/internal/util/retry/retry.go
@@ -20,20 +20,19 @@ import (
 
 func Do(ctx context.Context, fn func() error, opts ...Option) error {
 
-	c := NewDefaultConfig()
+	c := newDefaultConfig()
 
 	for _, opt := range opts {
 		opt(c)
 	}
-	el := make(ErrorList, c.attempts)
+	el := make(ErrorList, 0)
 
 	for i := uint(0); i < c.attempts; i++ {
 		if err := fn(); err != nil {
-			if s, ok := err.(InterruptError); ok {
-				return s.error
+			if ok := IsUncoverable(err); ok {
+				return err
 			}
-			// TODO early termination if this is unretriable error?
-			el[i] = err
+			el = append(el, err)
 
 			select {
 			case <-time.After(c.sleep):
@@ -52,9 +51,11 @@ func Do(ctx context.Context, fn func() error, opts ...Option) error {
 	return el
 }
 
+// ErrorList for print error log
 type ErrorList []error
 
 // TODO shouldn't print all retries, might be too much
+// Error method return an string representation of retry error list.
 func (el ErrorList) Error() string {
 	var builder strings.Builder
 	builder.WriteString("All attempts results:\n")
@@ -68,10 +69,17 @@ func (el ErrorList) Error() string {
 	return builder.String()
 }
 
-type InterruptError struct {
+type unrecoverableError struct {
 	error
 }
 
-func NoRetryError(err error) InterruptError {
-	return InterruptError{err}
+// Unrecoverable method wrap an error to unrecoverableError. This will make retry
+// quick return.
+func Unrecoverable(err error) error {
+	return unrecoverableError{err}
+}
+
+func IsUncoverable(err error) bool {
+	_, isUnrecoverable := err.(unrecoverableError)
+	return isUnrecoverable
 }
diff --git a/internal/util/retry/retry_test.go b/internal/util/retry/retry_test.go
index 20af596a83..0ba5a22f01 100644
--- a/internal/util/retry/retry_test.go
+++ b/internal/util/retry/retry_test.go
@@ -85,6 +85,20 @@ func TestAllError(t *testing.T) {
 	fmt.Println(err)
 }
 
+func TestUnRecoveryError(t *testing.T) {
+	attempts := 0
+	ctx := context.Background()
+
+	testFn := func() error {
+		attempts++
+		return Unrecoverable(fmt.Errorf("some error"))
+	}
+
+	err := Do(ctx, testFn, Attempts(3))
+	assert.NotNil(t, err)
+	assert.Equal(t, attempts, 1)
+}
+
 func TestContextDeadline(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
 	defer cancel()