Text updates, start of discerning 404 vs other errors

grafana · Sep 5, 2024 · 00e47f8 · 00e47f8
1 parent fccb91c
commit 00e47f8
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 4 deletions.
diff --git a/pkg/storage/chunk/client/aws/s3_storage_client.go b/pkg/storage/chunk/client/aws/s3_storage_client.go
@@ -124,7 +124,7 @@ func (cfg *S3Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
 
 	f.DurationVar(&cfg.BackoffConfig.MinBackoff, prefix+"s3.min-backoff", 100*time.Millisecond, "Minimum backoff time when s3 get Object")
 	f.DurationVar(&cfg.BackoffConfig.MaxBackoff, prefix+"s3.max-backoff", 3*time.Second, "Maximum backoff time when s3 get Object")
-	f.IntVar(&cfg.BackoffConfig.MaxRetries, prefix+"s3.max-retries", 5, "Maximum number of times to retry when s3 get Object")
+	f.IntVar(&cfg.BackoffConfig.MaxRetries, prefix+"s3.max-retries", 5, "Maximum number of times to retry for s3 GetObject or ObjectExists")
 }
 
 // Validate config and returns error on failure
@@ -312,7 +312,7 @@ func (a *S3ObjectClient) ObjectExists(ctx context.Context, objectKey string) (bo
 	retries := backoff.New(ctx, a.cfg.BackoffConfig)
 	for retries.Ongoing() {
 		if ctx.Err() != nil {
-			return false, errors.Wrap(ctx.Err(), "ctx related error during s3 getObject")
+			return false, errors.Wrap(ctx.Err(), "ctx related error during s3 objectExists")
 		}
 		lastErr = instrument.CollectedRequest(ctx, "S3.ObjectExists", s3RequestDuration, instrument.ErrorCode, func(ctx context.Context) error {
 			headObjectInput := &s3.HeadObjectInput{

diff --git a/pkg/storage/chunk/client/aws/s3_storage_client_test.go b/pkg/storage/chunk/client/aws/s3_storage_client_test.go
@@ -200,11 +200,13 @@ func Test_RetryLogic(t *testing.T) {
 	for _, tc := range []struct {
 		name       string
 		maxRetries int
+		exists     bool
 		do         func(c *S3ObjectClient) error
 	}{
 		{
 			"get object with retries",
 			3,
+			true,
 			func(c *S3ObjectClient) error {
 				_, _, err := c.GetObject(context.Background(), "foo")
 				return err
@@ -213,6 +215,16 @@ func Test_RetryLogic(t *testing.T) {
 		{
 			"object exists with retries",
 			3,
+			true,
+			func(c *S3ObjectClient) error {
+				_, err := c.ObjectExists(context.Background(), "foo")
+				return err
+			},
+		},
+		{
+			"object doesn't exist with retries",
+			3,
+			false,
 			func(c *S3ObjectClient) error {
 				_, err := c.ObjectExists(context.Background(), "foo")
 				return err
@@ -231,6 +243,9 @@ func Test_RetryLogic(t *testing.T) {
 					return RoundTripperFunc(func(req *http.Request) (*http.Response, error) {
 						// Increment the call counter
 						callNum := callCount.Inc()
+						if !tc.exists {
+							return nil, awserr.New(s3.ErrCodeNoSuchKey, "NoSuchKey", nil)
+						}
 
 						// Fail the first set of calls
 						if int(callNum) <= tc.maxRetries-1 {
@@ -248,8 +263,13 @@ func Test_RetryLogic(t *testing.T) {
 			}, hedging.Config{})
 			require.NoError(t, err)
 			err = tc.do(c)
-			require.NoError(t, err)
-			require.Equal(t, tc.maxRetries, int(callCount.Load()))
+			if tc.exists {
+				require.NoError(t, err)
+				require.Equal(t, tc.maxRetries, int(callCount.Load()))
+			} else {
+				//require.True(t, errors.As(err, &notFoundErr))
+				require.Equal(t, 1, int(callCount.Load()))
+			}
 		})
 	}
 }