From 0da9ab0831540569dc04c0a23437b084b1564e15 Mon Sep 17 00:00:00 2001 From: Chris Cotter Date: Thu, 11 Aug 2022 18:00:47 -0400 Subject: [PATCH] feat(storage): export ShouldRetry (#6370) Export the default func to determine whether an error is retryable. This makes it easier for users to use the WithErrorFunc option without copying a lot of code. Fixes #6362 --- storage/grpc_client.go | 6 +++--- storage/invoke.go | 15 ++++++++++++--- storage/invoke_test.go | 2 +- storage/storage.go | 7 ++++--- storage/storage_test.go | 28 ++++++++++++++-------------- 5 files changed, 34 insertions(+), 24 deletions(-) diff --git a/storage/grpc_client.go b/storage/grpc_client.go index 8a08e35ec2b7..09fddbdf1f78 100644 --- a/storage/grpc_client.go +++ b/storage/grpc_client.go @@ -1379,7 +1379,7 @@ func (r *gRPCReader) Close() error { // an attempt to reopen the stream. func (r *gRPCReader) recv() (*storagepb.ReadObjectResponse, error) { msg, err := r.stream.Recv() - if err != nil && shouldRetry(err) { + if err != nil && ShouldRetry(err) { // This will "close" the existing stream and immediately attempt to // reopen the stream, but will backoff if further attempts are necessary. // Reopening the stream Recvs the first message, so if retrying is @@ -1559,7 +1559,7 @@ func (w *gRPCWriter) uploadBuffer(recvd int, start int64, doneReading bool) (*st // resend the entire buffer via a new stream. // If not retriable, falling through will return the error received // from closing the stream. - if shouldRetry(err) { + if ShouldRetry(err) { sent = 0 finishWrite = false // TODO: Add test case for failure modes of querying progress. @@ -1590,7 +1590,7 @@ func (w *gRPCWriter) uploadBuffer(recvd int, start int64, doneReading bool) (*st // resend the entire buffer via a new stream. // If not retriable, falling through will return the error received // from closing the stream. - if shouldRetry(err) { + if ShouldRetry(err) { sent = 0 finishWrite = false offset, err = w.determineOffset(start) diff --git a/storage/invoke.go b/storage/invoke.go index d0f0dd8d1718..810d64285d0c 100644 --- a/storage/invoke.go +++ b/storage/invoke.go @@ -57,7 +57,7 @@ func run(ctx context.Context, call func() error, retry *retryConfig, isIdempoten bo.Initial = retry.backoff.Initial bo.Max = retry.backoff.Max } - var errorFunc func(err error) bool = shouldRetry + var errorFunc func(err error) bool = ShouldRetry if retry.shouldRetry != nil { errorFunc = retry.shouldRetry } @@ -89,7 +89,16 @@ func setRetryHeaderGRPC(_ context.Context) func(string, int) { } } -func shouldRetry(err error) bool { +// ShouldRetry returns true if an error is retryable, based on best practice +// guidance from GCS. See +// https://cloud.google.com/storage/docs/retry-strategy#go for more information +// on what errors are considered retryable. +// +// If you would like to customize retryable errors, use the WithErrorFunc to +// supply a RetryOption to your library calls. For example, to retry additional +// errors, you can write a custom func that wraps ShouldRetry and also specifies +// additional errors that should return true. +func ShouldRetry(err error) bool { if err == nil { return false } @@ -131,7 +140,7 @@ func shouldRetry(err error) bool { } // Unwrap is only supported in go1.13.x+ if e, ok := err.(interface{ Unwrap() error }); ok { - return shouldRetry(e.Unwrap()) + return ShouldRetry(e.Unwrap()) } return false } diff --git a/storage/invoke_test.go b/storage/invoke_test.go index 02139228592c..bb27526bcb10 100644 --- a/storage/invoke_test.go +++ b/storage/invoke_test.go @@ -306,7 +306,7 @@ func TestShouldRetry(t *testing.T) { }, } { t.Run(test.desc, func(s *testing.T) { - got := shouldRetry(test.inputErr) + got := ShouldRetry(test.inputErr) if got != test.shouldRetry { s.Errorf("got %v, want %v", got, test.shouldRetry) diff --git a/storage/storage.go b/storage/storage.go index 3d42caa42894..77c7475277cb 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -1875,8 +1875,8 @@ func (ws *withPolicy) apply(config *retryConfig) { // WithErrorFunc allows users to pass a custom function to the retryer. Errors // will be retried if and only if `shouldRetry(err)` returns true. -// By default, the following errors are retried (see invoke.go for the default -// shouldRetry function): +// By default, the following errors are retried (see ShouldRetry for the default +// function): // // - HTTP responses with codes 408, 429, 502, 503, and 504. // @@ -1887,7 +1887,8 @@ func (ws *withPolicy) apply(config *retryConfig) { // - Wrapped versions of these errors. // // This option can be used to retry on a different set of errors than the -// default. +// default. Users can use the default ShouldRetry function inside their custom +// function if they only want to make minor modifications to default behavior. func WithErrorFunc(shouldRetry func(err error) bool) RetryOption { return &withErrorFunc{ shouldRetry: shouldRetry, diff --git a/storage/storage_test.go b/storage/storage_test.go index c1bce60c070d..18a19f4c2559 100644 --- a/storage/storage_test.go +++ b/storage/storage_test.go @@ -1139,10 +1139,10 @@ func TestRetryer(t *testing.T) { name: "object retryer configures retry", objectOptions: []RetryOption{ WithPolicy(RetryAlways), - WithErrorFunc(shouldRetry), + WithErrorFunc(ShouldRetry), }, want: &retryConfig{ - shouldRetry: shouldRetry, + shouldRetry: ShouldRetry, policy: RetryAlways, }, }, @@ -1155,7 +1155,7 @@ func TestRetryer(t *testing.T) { Multiplier: 6, }), WithPolicy(RetryAlways), - WithErrorFunc(shouldRetry), + WithErrorFunc(ShouldRetry), }, want: &retryConfig{ backoff: &gax.Backoff{ @@ -1163,7 +1163,7 @@ func TestRetryer(t *testing.T) { Max: time.Hour, Multiplier: 6, }, - shouldRetry: shouldRetry, + shouldRetry: ShouldRetry, policy: RetryAlways, }, }, @@ -1176,7 +1176,7 @@ func TestRetryer(t *testing.T) { Multiplier: 6, }), WithPolicy(RetryAlways), - WithErrorFunc(shouldRetry), + WithErrorFunc(ShouldRetry), }, want: &retryConfig{ backoff: &gax.Backoff{ @@ -1184,7 +1184,7 @@ func TestRetryer(t *testing.T) { Max: time.Hour, Multiplier: 6, }, - shouldRetry: shouldRetry, + shouldRetry: ShouldRetry, policy: RetryAlways, }, }, @@ -1195,11 +1195,11 @@ func TestRetryer(t *testing.T) { }, objectOptions: []RetryOption{ WithPolicy(RetryNever), - WithErrorFunc(shouldRetry), + WithErrorFunc(ShouldRetry), }, want: &retryConfig{ policy: RetryNever, - shouldRetry: shouldRetry, + shouldRetry: ShouldRetry, }, }, { @@ -1209,11 +1209,11 @@ func TestRetryer(t *testing.T) { }, objectOptions: []RetryOption{ WithPolicy(RetryNever), - WithErrorFunc(shouldRetry), + WithErrorFunc(ShouldRetry), }, want: &retryConfig{ policy: RetryNever, - shouldRetry: shouldRetry, + shouldRetry: ShouldRetry, }, }, { @@ -1231,11 +1231,11 @@ func TestRetryer(t *testing.T) { Initial: time.Nanosecond, Max: time.Microsecond, }), - WithErrorFunc(shouldRetry), + WithErrorFunc(ShouldRetry), }, want: &retryConfig{ policy: RetryAlways, - shouldRetry: shouldRetry, + shouldRetry: ShouldRetry, backoff: &gax.Backoff{ Initial: time.Nanosecond, Max: time.Microsecond, @@ -1268,7 +1268,7 @@ func TestRetryer(t *testing.T) { name: "object retryer does not override bucket retryer if option is not set", bucketOptions: []RetryOption{ WithPolicy(RetryNever), - WithErrorFunc(shouldRetry), + WithErrorFunc(ShouldRetry), }, objectOptions: []RetryOption{ WithBackoff(gax.Backoff{ @@ -1278,7 +1278,7 @@ func TestRetryer(t *testing.T) { }, want: &retryConfig{ policy: RetryNever, - shouldRetry: shouldRetry, + shouldRetry: ShouldRetry, backoff: &gax.Backoff{ Initial: time.Nanosecond, Max: time.Second,