Merge pull request #159 from aknuds1/arve/revert-not-return-error

Revert "Merge pull request #103 from thanos-io/do-not-return-error-when-expected"

Author: yeya24

CHANGELOG.md

@@ -68,6 +68,4 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re
 - [#71](https://github.com/thanos-io/objstore/pull/71) Replace method `IsCustomerManagedKeyError` for a more generic `IsAccessDeniedErr` on the bucket interface.
 - [#89](https://github.com/thanos-io/objstore/pull/89) GCS: Upgrade cloud.google.com/go/storage version to `v1.35.1`.
 - [#123](https://github.com/thanos-io/objstore/pull/123) *: Upgrade minio-go version to `v7.0.71`.
-- [#103](https://github.com/thanos-io/objstore/pull/103) *: Don't return error in metric bucket if the error is expected.
-
 ### Removed

objstore.go

@@ -633,9 +633,7 @@ func (b *metricBucket) Iter(ctx context.Context, dir string, f func(string) erro
 
 	err := b.bkt.Iter(ctx, dir, f, options...)
 	if err != nil {
-		if b.metrics.isOpFailureExpected(err) {
-			err = nil
-		} else if ctx.Err() != context.Canceled {
+		if !b.metrics.isOpFailureExpected(err) && ctx.Err() != context.Canceled {
 			b.metrics.opsFailures.WithLabelValues(op).Inc()
 		}
 	}
@@ -651,9 +649,7 @@ func (b *metricBucket) IterWithAttributes(ctx context.Context, dir string, f fun
 
 	err := b.bkt.IterWithAttributes(ctx, dir, f, options...)
 	if err != nil {
-		if b.metrics.isOpFailureExpected(err) {
-			err = nil
-		} else if ctx.Err() != context.Canceled {
+		if !b.metrics.isOpFailureExpected(err) && ctx.Err() != context.Canceled {
 			b.metrics.opsFailures.WithLabelValues(op).Inc()
 		}
 	}
@@ -672,9 +668,7 @@ func (b *metricBucket) Attributes(ctx context.Context, name string) (ObjectAttri
 	start := time.Now()
 	attrs, err := b.bkt.Attributes(ctx, name)
 	if err != nil {
-		if b.metrics.isOpFailureExpected(err) {
-			err = nil
-		} else if ctx.Err() != context.Canceled {
+		if !b.metrics.isOpFailureExpected(err) && ctx.Err() != context.Canceled {
 			b.metrics.opsFailures.WithLabelValues(op).Inc()
 		}
 		return attrs, err
@@ -691,9 +685,7 @@ func (b *metricBucket) Get(ctx context.Context, name string) (io.ReadCloser, err
 
 	rc, err := b.bkt.Get(ctx, name)
 	if err != nil {
-		if b.metrics.isOpFailureExpected(err) {
-			err = nil
-		} else if ctx.Err() != context.Canceled {
+		if !b.metrics.isOpFailureExpected(err) && ctx.Err() != context.Canceled {
 			b.metrics.opsFailures.WithLabelValues(op).Inc()
 		}
 		b.metrics.opsDuration.WithLabelValues(op).Observe(time.Since(start).Seconds())
@@ -720,9 +712,7 @@ func (b *metricBucket) GetRange(ctx context.Context, name string, off, length in
 
 	rc, err := b.bkt.GetRange(ctx, name, off, length)
 	if err != nil {
-		if b.metrics.isOpFailureExpected(err) {
-			err = nil
-		} else if ctx.Err() != context.Canceled {
+		if !b.metrics.isOpFailureExpected(err) && ctx.Err() != context.Canceled {
 			b.metrics.opsFailures.WithLabelValues(op).Inc()
 		}
 		b.metrics.opsDuration.WithLabelValues(op).Observe(time.Since(start).Seconds())
@@ -748,9 +738,7 @@ func (b *metricBucket) Exists(ctx context.Context, name string) (bool, error) {
 	start := time.Now()
 	ok, err := b.bkt.Exists(ctx, name)
 	if err != nil {
-		if b.metrics.isOpFailureExpected(err) {
-			err = nil
-		} else if ctx.Err() != context.Canceled {
+		if !b.metrics.isOpFailureExpected(err) && ctx.Err() != context.Canceled {
 			b.metrics.opsFailures.WithLabelValues(op).Inc()
 		}
 		return false, err
@@ -779,9 +767,7 @@ func (b *metricBucket) Upload(ctx context.Context, name string, r io.Reader) err
 	defer trc.Close()
 	err := b.bkt.Upload(ctx, name, trc)
 	if err != nil {
-		if b.metrics.isOpFailureExpected(err) {
-			err = nil
-		} else if ctx.Err() != context.Canceled {
+		if !b.metrics.isOpFailureExpected(err) && ctx.Err() != context.Canceled {
 			b.metrics.opsFailures.WithLabelValues(op).Inc()
 		}
 		return err
@@ -797,9 +783,7 @@ func (b *metricBucket) Delete(ctx context.Context, name string) error {
 
 	start := time.Now()
 	if err := b.bkt.Delete(ctx, name); err != nil {
-		if b.metrics.isOpFailureExpected(err) {
-			err = nil
-		} else if ctx.Err() != context.Canceled {
+		if !b.metrics.isOpFailureExpected(err) && ctx.Err() != context.Canceled {
 			b.metrics.opsFailures.WithLabelValues(op).Inc()
 		}
 		return err

objstore_test.go

@@ -28,7 +28,7 @@ func TestMetricBucket_Close(t *testing.T) {
 	testutil.Equals(t, 7, promtest.CollectAndCount(bkt.metrics.opsFailures))
 	testutil.Equals(t, 7, promtest.CollectAndCount(bkt.metrics.opsDuration))
 
-	AcceptanceTest(t, bkt.WithExpectedErrs(bkt.IsObjNotFoundErr), true)
+	AcceptanceTest(t, bkt.WithExpectedErrs(bkt.IsObjNotFoundErr))
 	testutil.Equals(t, float64(9), promtest.ToFloat64(bkt.metrics.ops.WithLabelValues(OpIter)))
 	testutil.Equals(t, float64(2), promtest.ToFloat64(bkt.metrics.ops.WithLabelValues(OpAttributes)))
 	testutil.Equals(t, float64(3), promtest.ToFloat64(bkt.metrics.ops.WithLabelValues(OpGet)))
@@ -51,7 +51,7 @@ func TestMetricBucket_Close(t *testing.T) {
 
 	// Clear bucket, but don't clear metrics to ensure we use same.
 	bkt.bkt = NewInMemBucket()
-	AcceptanceTestWithoutNotFoundErr(t, bkt)
+	AcceptanceTest(t, bkt)
 	testutil.Equals(t, float64(18), promtest.ToFloat64(bkt.metrics.ops.WithLabelValues(OpIter)))
 	testutil.Equals(t, float64(4), promtest.ToFloat64(bkt.metrics.ops.WithLabelValues(OpAttributes)))
 	testutil.Equals(t, float64(6), promtest.ToFloat64(bkt.metrics.ops.WithLabelValues(OpGet)))
@@ -537,54 +537,6 @@ func TestDownloadDir_CleanUp(t *testing.T) {
 	testutil.Assert(t, os.IsNotExist(err))
 }
 
-func TestBucketExpectedErrNoReturnError(t *testing.T) {
-	expectedErr := errors.New("test error")
-
-	bucket := WrapWithMetrics(&mockBucket{
-		get: func(_ context.Context, _ string) (io.ReadCloser, error) {
-			return nil, expectedErr
-		},
-		getRange: func(ctx context.Context, name string, off, length int64) (io.ReadCloser, error) {
-			return nil, expectedErr
-		},
-		upload: func(ctx context.Context, name string, r io.Reader) error {
-			return expectedErr
-		},
-		iter: func(ctx context.Context, dir string, f func(string) error, options ...IterOption) error {
-			return expectedErr
-		},
-		attributes: func(ctx context.Context, name string) (ObjectAttributes, error) {
-			return ObjectAttributes{}, expectedErr
-		},
-		exists: func(ctx context.Context, name string) (bool, error) {
-			return false, expectedErr
-		},
-	}, nil, "").WithExpectedErrs(func(err error) bool {
-		return errors.Is(err, expectedErr)
-	})
-
-	// Expect no error to be returned since the error is expected.
-	_, err := bucket.Get(context.Background(), "")
-	testutil.Ok(t, err)
-
-	_, err = bucket.GetRange(context.Background(), "", 1, 2)
-	testutil.Ok(t, err)
-
-	err = bucket.Upload(context.Background(), "", nil)
-	testutil.Ok(t, err)
-
-	err = bucket.Iter(context.Background(), "", func(s string) error {
-		return nil
-	})
-	testutil.Ok(t, err)
-
-	_, err = bucket.Exists(context.Background(), "")
-	testutil.Ok(t, err)
-
-	_, err = bucket.Attributes(context.Background(), "")
-	testutil.Ok(t, err)
-}
-
 // unreliableBucket implements Bucket and returns an error on every n-th Get.
 type unreliableBucket struct {
 	Bucket
@@ -618,12 +570,9 @@ func (r *mockReader) Close() error {
 type mockBucket struct {
 	Bucket
 
-	upload     func(ctx context.Context, name string, r io.Reader) error
-	get        func(ctx context.Context, name string) (io.ReadCloser, error)
-	getRange   func(ctx context.Context, name string, off, length int64) (io.ReadCloser, error)
-	iter       func(ctx context.Context, dir string, f func(string) error, options ...IterOption) error
-	attributes func(ctx context.Context, name string) (ObjectAttributes, error)
-	exists     func(ctx context.Context, name string) (bool, error)
+	upload   func(ctx context.Context, name string, r io.Reader) error
+	get      func(ctx context.Context, name string) (io.ReadCloser, error)
+	getRange func(ctx context.Context, name string, off, length int64) (io.ReadCloser, error)
 }
 
 func (b *mockBucket) Upload(ctx context.Context, name string, r io.Reader) error {
@@ -647,27 +596,6 @@ func (b *mockBucket) GetRange(ctx context.Context, name string, off, length int6
 	return nil, errors.New("GetRange has not been mocked")
 }
 
-func (b *mockBucket) Iter(ctx context.Context, dir string, f func(string) error, options ...IterOption) error {
-	if b.iter != nil {
-		return b.iter(ctx, dir, f, options...)
-	}
-	return errors.New("Iter has not been mocked")
-}
-
-func (b *mockBucket) Exists(ctx context.Context, name string) (bool, error) {
-	if b.exists != nil {
-		return b.exists(ctx, name)
-	}
-	return false, errors.New("Exists has not been mocked")
-}
-
-func (b *mockBucket) Attributes(ctx context.Context, name string) (ObjectAttributes, error) {
-	if b.attributes != nil {
-		return b.attributes(ctx, name)
-	}
-	return ObjectAttributes{}, errors.New("Attributes has not been mocked")
-}
-
 func Test_TryToGetSizeLimitedReader(t *testing.T) {
 	b := &bytes.Buffer{}
 	r := io.LimitReader(b, 1024)

objtesting/acceptance_e2e_test.go

@@ -14,5 +14,5 @@ import (
 // NOTE: This test assumes strong consistency, but in the same way it does not guarantee that if it passes, the
 // used object store is strongly consistent.
 func TestObjStore_AcceptanceTest_e2e(t *testing.T) {
-	ForeachStore(t, objstore.AcceptanceTestWithoutNotFoundErr)
+	ForeachStore(t, objstore.AcceptanceTest)
 }

prefixed_bucket_test.go

@@ -23,7 +23,7 @@ func TestPrefixedBucket_Acceptance(t *testing.T) {
 		"someprefix"}
 
 	for _, prefix := range prefixes {
-		AcceptanceTestWithoutNotFoundErr(t, NewPrefixedBucket(NewInMemBucket(), prefix))
+		AcceptanceTest(t, NewPrefixedBucket(NewInMemBucket(), prefix))
 		UsesPrefixTest(t, NewInMemBucket(), prefix)
 	}
 }

testing.go

@@ -80,32 +80,24 @@ func (b noopInstrumentedBucket) ReaderWithExpectedErrs(IsOpFailureExpectedFunc)
 	return b
 }
 
-func AcceptanceTestWithoutNotFoundErr(t *testing.T, bkt Bucket) {
-	AcceptanceTest(t, bkt, false)
-}
-
-func AcceptanceTest(t *testing.T, bkt Bucket, expectedNotFoundErr bool) {
+func AcceptanceTest(t *testing.T, bkt Bucket) {
 	ctx := context.Background()
 
 	_, err := bkt.Get(ctx, "")
 	testutil.NotOk(t, err)
 	testutil.Assert(t, !bkt.IsObjNotFoundErr(err), "expected user error got not found %s", err)
 
 	_, err = bkt.Get(ctx, "id1/obj_1.some")
-	if !expectedNotFoundErr {
-		testutil.NotOk(t, err)
-		testutil.Assert(t, bkt.IsObjNotFoundErr(err), "expected not found error got %s", err)
-	}
+	testutil.NotOk(t, err)
+	testutil.Assert(t, bkt.IsObjNotFoundErr(err), "expected not found error got %s", err)
 
 	ok, err := bkt.Exists(ctx, "id1/obj_1.some")
 	testutil.Ok(t, err)
 	testutil.Assert(t, !ok, "expected not exits")
 
 	_, err = bkt.Attributes(ctx, "id1/obj_1.some")
-	if !expectedNotFoundErr {
-		testutil.NotOk(t, err)
-		testutil.Assert(t, bkt.IsObjNotFoundErr(err), "expected not found error got %s", err)
-	}
+	testutil.NotOk(t, err)
+	testutil.Assert(t, bkt.IsObjNotFoundErr(err), "expected not found error but got %s", err)
 
 	// Upload first object.
 	testutil.Ok(t, bkt.Upload(ctx, "id1/obj_1.some", strings.NewReader("@test-data@")))