From bc2e25bd211a77c5a9c6a4f9b23b2120be529234 Mon Sep 17 00:00:00 2001 From: Arve Knudsen Date: Mon, 16 Dec 2024 10:00:08 +0100 Subject: [PATCH] Revert "Merge pull request #103 from thanos-io/do-not-return-error-when-expected" This reverts commit d69df7208cbace59b1e25d0284b98640045e5749, reversing changes made to 8d266b99071651d4211e4a64fcdfb66da9841929. Signed-off-by: Arve Knudsen --- CHANGELOG.md | 2 - objstore.go | 32 +++--------- objstore_test.go | 82 ++----------------------------- objtesting/acceptance_e2e_test.go | 2 +- prefixed_bucket_test.go | 2 +- testing.go | 18 ++----- 6 files changed, 20 insertions(+), 118 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c068dbb0..d0779d62 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/objstore.go b/objstore.go index 0bdd3f45..86ecfa26 100644 --- a/objstore.go +++ b/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 diff --git a/objstore_test.go b/objstore_test.go index 79c94eed..b1f82922 100644 --- a/objstore_test.go +++ b/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) diff --git a/objtesting/acceptance_e2e_test.go b/objtesting/acceptance_e2e_test.go index 611dd382..e162cee5 100644 --- a/objtesting/acceptance_e2e_test.go +++ b/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) } diff --git a/prefixed_bucket_test.go b/prefixed_bucket_test.go index 230b54cb..6252e05d 100644 --- a/prefixed_bucket_test.go +++ b/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) } } diff --git a/testing.go b/testing.go index bd6fe856..80f1e198 100644 --- a/testing.go +++ b/testing.go @@ -80,11 +80,7 @@ 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, "") @@ -92,20 +88,16 @@ func AcceptanceTest(t *testing.T, bkt Bucket, expectedNotFoundErr bool) { 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@")))