From 1d1fe4da7e7dda6448e86fca3e1d94bb264fca68 Mon Sep 17 00:00:00 2001 From: Chris Cotter Date: Thu, 9 Dec 2021 15:26:05 -0500 Subject: [PATCH 1/4] feat(storage): configurable retries for uploads Depends on merge and release of googleapis/google-api-go-client#1324 --- storage/writer.go | 19 +++++++++++++++++++ storage/writer_test.go | 2 +- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/storage/writer.go b/storage/writer.go index ec55e4a3ba4..dd468c5bf9e 100644 --- a/storage/writer.go +++ b/storage/writer.go @@ -172,6 +172,25 @@ func (w *Writer) open() error { // call to set up the upload as well as calls to upload individual chunks // for a resumable upload (as long as the chunk size is non-zero). Hence // there is no need to add retries here. + + // Retry only when the operation is idempotent or the retry policy is RetryAlways. + var isIdempotent bool + if w.o.conds != nil && (w.o.conds.GenerationMatch >= 0 || w.o.conds.DoesNotExist == true) { + isIdempotent = true + } + var useRetry bool + if (w.o.retry == nil || w.o.retry.policy == RetryIdempotent) && isIdempotent { + useRetry = true + } else if w.o.retry != nil && w.o.retry.policy == RetryAlways { + useRetry = true + } + if useRetry { + if w.o.retry != nil { + call.WithRetry(w.o.retry.backoff, w.o.retry.shouldRetry) + } else { + call.WithRetry(nil, nil) + } + } resp, err = call.Do() } if err != nil { diff --git a/storage/writer_test.go b/storage/writer_test.go index 7ac9b7ef509..2537da9a633 100644 --- a/storage/writer_test.go +++ b/storage/writer_test.go @@ -36,7 +36,7 @@ func TestErrorOnObjectsInsertCall(t *testing.T) { doWrite := func(mt *mockTransport) *Writer { client := mockClient(t, mt) - wc := client.Bucket("bucketname").Object("filename1").NewWriter(ctx) + wc := client.Bucket("bucketname").Object("filename1").If(Conditions{DoesNotExist: true}).NewWriter(ctx) wc.ContentType = "text/plain" // We can't check that the Write fails, since it depends on the write to the From 05151161c848f4e5bae0e71167815a358e56d931 Mon Sep 17 00:00:00 2001 From: Chris Cotter Date: Mon, 13 Dec 2021 12:08:03 -0500 Subject: [PATCH 2/4] update idempotency condition --- storage/writer.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/storage/writer.go b/storage/writer.go index dd468c5bf9e..51d8b63779b 100644 --- a/storage/writer.go +++ b/storage/writer.go @@ -174,10 +174,7 @@ func (w *Writer) open() error { // there is no need to add retries here. // Retry only when the operation is idempotent or the retry policy is RetryAlways. - var isIdempotent bool - if w.o.conds != nil && (w.o.conds.GenerationMatch >= 0 || w.o.conds.DoesNotExist == true) { - isIdempotent = true - } + isIdempotent := w.o.conds != nil && (w.o.conds.GenerationMatch >= 0 || w.o.conds.DoesNotExist == true) var useRetry bool if (w.o.retry == nil || w.o.retry.policy == RetryIdempotent) && isIdempotent { useRetry = true From 05976fd4506d5e8a1b535d3e63c1887f5d4520e4 Mon Sep 17 00:00:00 2001 From: Chris Cotter Date: Tue, 14 Dec 2021 01:02:00 -0500 Subject: [PATCH 3/4] update emulator script --- storage/emulator_test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/storage/emulator_test.sh b/storage/emulator_test.sh index 092fecf0f4f..4eeb890c5c0 100755 --- a/storage/emulator_test.sh +++ b/storage/emulator_test.sh @@ -57,7 +57,6 @@ trap cleanup EXIT # TODO: move to passing once fixed FAILING=( "buckets.setIamPolicy" - "objects.insert" "hmacKey.update" ) # TODO: remove regex once all tests are passing @@ -74,6 +73,7 @@ PASSING=( "buckets.list" "buckets.lockRetentionPolicy" "objects.copy" "objects.get" + "objects.insert" "objects.list" "objects.delete" "objects.update" From 58baa0f3f47e38763cee6859c997c28fd83914a9 Mon Sep 17 00:00:00 2001 From: Chris Cotter Date: Thu, 23 Dec 2021 12:22:04 -0500 Subject: [PATCH 4/4] remove regex for passing tests --- storage/emulator_test.sh | 56 +--------------------------------------- 1 file changed, 1 insertion(+), 55 deletions(-) diff --git a/storage/emulator_test.sh b/storage/emulator_test.sh index 5f973341eb1..865a5600818 100755 --- a/storage/emulator_test.sh +++ b/storage/emulator_test.sh @@ -55,59 +55,5 @@ function cleanup() { } trap cleanup EXIT -# TODO: remove regex once all tests are passing -# Unfortunately, there is no simple way to skip specific tests (see https://github.com/golang/go/issues/41583) -# Therefore, we have to simply run all the specific tests we know pass -PASSING=( "buckets.list" - "buckets.insert" - "buckets.get" - "buckets.delete" - "buckets.update" - "buckets.patch" - "buckets.getIamPolicy" - "buckets.setIamPolicy" - "buckets.testIamPermissions" - "buckets.lockRetentionPolicy" - "objects.copy" - "objects.get" - "objects.insert" - "objects.list" - "objects.delete" - "objects.update" - "objects.patch" - "objects.compose" - "objects.rewrite" - "serviceaccount.get" - "hmacKey.get" - "hmacKey.list" - "hmacKey.create" - "hmacKey.delete" - "hmacKey.update" - "notifications.list" - "notifications.create" - "notifications.get" - "notifications.delete" - "object_acl.insert" - "object_acl.get" - "object_acl.list" - "object_acl.patch" - "object_acl.update" - "object_acl.delete" - "default_object_acl.insert" - "default_object_acl.get" - "default_object_acl.list" - "default_object_acl.patch" - "default_object_acl.update" - "default_object_acl.delete" - "bucket_acl.insert" - "bucket_acl.get" - "bucket_acl.list" - "bucket_acl.patch" - "bucket_acl.update" - "bucket_acl.delete" - ) -TEMP=${PASSING[@]} -PASSING_REGEX=${TEMP// /|} - # Run tests -go test -v -timeout 10m ./ -run="TestRetryConformance/($PASSING_REGEX)-" -short 2>&1 | tee -a sponge_log.log +go test -v -timeout 10m ./ -run="TestRetryConformance" -short 2>&1 | tee -a sponge_log.log