Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(storage): add maxAttempts RetryOption #9215

Merged
merged 26 commits into from
Jan 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
7046ee5
adding max-retry-count to stop retries
Tulsishah Dec 29, 2023
92ca12a
Merge branch 'googleapis:main' into main
Tulsishah Jan 4, 2024
303b8e8
adding option to configure max-retry-count
Tulsishah Jan 4, 2024
fbb30ff
Replacing WithMaxRetry to WithMaxAttempts
Tulsishah Jan 5, 2024
c154295
git test failing
Tulsishah Jan 5, 2024
0c94854
git test failing
Tulsishah Jan 5, 2024
03aa6fe
adding unit tests for invoke.go
Tulsishah Jan 5, 2024
e060d3a
adding unit tests for invoke.go
Tulsishah Jan 5, 2024
f3ac62f
adding unit tests for invoke.go
Tulsishah Jan 5, 2024
17890c9
fixing comment
Tulsishah Jan 6, 2024
ca327a0
fixing git test
Tulsishah Jan 7, 2024
8527d68
Merge branch 'main' into configure_max_retry_count
Tulsishah Jan 7, 2024
a4912a8
fixing comments
Tulsishah Jan 11, 2024
ffa763d
Merge remote-tracking branch 'origin/configure_max_retry_count' into …
Tulsishah Jan 11, 2024
f604a31
adding more unit tests
Tulsishah Jan 11, 2024
af39147
small fix
Tulsishah Jan 11, 2024
c4e0c1f
Merge branch 'main' into configure_max_retry_count
Tulsishah Jan 11, 2024
1877bb4
converting int to pointer
Tulsishah Jan 11, 2024
ca59001
removing logic of settiing retrynever when maxAttempts equals to 0
Tulsishah Jan 15, 2024
de87569
removing unnecessay tests
Tulsishah Jan 15, 2024
57e1ed7
Merge branch 'main' into configure_max_retry_count
Tulsishah Jan 15, 2024
63eca5f
Merge branch 'main' into configure_max_retry_count
tritone Jan 17, 2024
253d65e
doc update
Tulsishah Jan 17, 2024
6b3ed1a
Merge remote-tracking branch 'origin/configure_max_retry_count' into …
Tulsishah Jan 17, 2024
889808f
doc update
Tulsishah Jan 17, 2024
c155efe
Merge branch 'main' into configure_max_retry_count
tritone Jan 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 additions & 0 deletions storage/bucket_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1071,6 +1071,7 @@ func TestBucketRetryer(t *testing.T) {
Multiplier: 3,
}),
WithPolicy(RetryAlways),
WithMaxAttempts(5),
WithErrorFunc(func(err error) bool { return false }))
},
want: &retryConfig{
Expand All @@ -1080,6 +1081,7 @@ func TestBucketRetryer(t *testing.T) {
Multiplier: 3,
},
policy: RetryAlways,
maxAttempts: expectedAttempts(5),
shouldRetry: func(err error) bool { return false },
},
},
Expand All @@ -1105,6 +1107,15 @@ func TestBucketRetryer(t *testing.T) {
policy: RetryNever,
},
},
{
name: "set max retry attempts only",
call: func(b *BucketHandle) *BucketHandle {
return b.Retryer(WithMaxAttempts(5))
},
want: &retryConfig{
maxAttempts: expectedAttempts(5),
},
},
{
name: "set ErrorFunc only",
call: func(b *BucketHandle) *BucketHandle {
Expand Down
3 changes: 3 additions & 0 deletions storage/invoke.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ func run(ctx context.Context, call func(ctx context.Context) error, retry *retry
return internal.Retry(ctx, bo, func() (stop bool, err error) {
ctxWithHeaders := setInvocationHeaders(ctx, invocationID, attempts)
err = call(ctxWithHeaders)
if retry.maxAttempts != nil && attempts >= *retry.maxAttempts {
return true, err
}
attempts++
return !errorFunc(err), err
})
Expand Down
65 changes: 63 additions & 2 deletions storage/invoke_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@ func TestInvoke(t *testing.T) {
expectFinalErr: false,
},
{

desc: "non-idempotent retriable error retried when policy is RetryAlways",
count: 2,
initialErr: &googleapi.Error{Code: 500},
Expand All @@ -132,7 +131,6 @@ func TestInvoke(t *testing.T) {
retry: &retryConfig{policy: RetryAlways},
expectFinalErr: false,
},

{
desc: "non-retriable error retried with custom fn",
count: 2,
Expand Down Expand Up @@ -173,6 +171,65 @@ func TestInvoke(t *testing.T) {
},
expectFinalErr: false,
},
{
desc: "non-idempotent retriable error retried when policy is RetryAlways till maxAttempts",
count: 4,
initialErr: &googleapi.Error{Code: 500},
finalErr: nil,
isIdempotentValue: false,
retry: &retryConfig{policy: RetryAlways, maxAttempts: expectedAttempts(2)},
expectFinalErr: false,
},
{
desc: "non-idempotent retriable error not retried when policy is RetryNever with maxAttempts set",
count: 4,
initialErr: &googleapi.Error{Code: 500},
finalErr: nil,
isIdempotentValue: false,
retry: &retryConfig{policy: RetryNever, maxAttempts: expectedAttempts(2)},
expectFinalErr: false,
},
{
desc: "non-retriable error retried with custom fn till maxAttempts",
count: 4,
initialErr: io.ErrNoProgress,
finalErr: nil,
isIdempotentValue: true,
retry: &retryConfig{
shouldRetry: func(err error) bool {
return err == io.ErrNoProgress
},
maxAttempts: expectedAttempts(2),
},
expectFinalErr: false,
Tulsishah marked this conversation as resolved.
Show resolved Hide resolved
},
{
desc: "non-idempotent retriable error retried when policy is RetryAlways till maxAttempts where count equals to maxAttempts-1",
count: 3,
initialErr: &googleapi.Error{Code: 500},
finalErr: nil,
isIdempotentValue: false,
retry: &retryConfig{policy: RetryAlways, maxAttempts: expectedAttempts(4)},
expectFinalErr: true,
},
{
desc: "non-idempotent retriable error retried when policy is RetryAlways till maxAttempts where count equals to maxAttempts",
count: 4,
initialErr: &googleapi.Error{Code: 500},
finalErr: nil,
isIdempotentValue: true,
retry: &retryConfig{policy: RetryAlways, maxAttempts: expectedAttempts(4)},
expectFinalErr: false,
},
{
desc: "non-idempotent retriable error not retried when policy is RetryAlways with maxAttempts equals to zero",
count: 4,
initialErr: &googleapi.Error{Code: 500},
finalErr: nil,
isIdempotentValue: true,
retry: &retryConfig{maxAttempts: expectedAttempts(0), policy: RetryAlways},
expectFinalErr: false,
},
} {
t.Run(test.desc, func(s *testing.T) {
counter := 0
Expand Down Expand Up @@ -203,6 +260,10 @@ func TestInvoke(t *testing.T) {
if !test.expectFinalErr {
wantAttempts = 1
}
if test.retry != nil && test.retry.maxAttempts != nil && *test.retry.maxAttempts != 0 && test.retry.policy != RetryNever {
wantAttempts = *test.retry.maxAttempts
}

wantClientHeader := strings.ReplaceAll(initialClientHeader, "gccl-attempt-count/1", fmt.Sprintf("gccl-attempt-count/%v", wantAttempts))
if gotClientHeader != wantClientHeader {
t.Errorf("case %q, retry header:\ngot %v\nwant %v", test.desc, gotClientHeader, wantClientHeader)
Expand Down
22 changes: 22 additions & 0 deletions storage/storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -2076,6 +2076,26 @@ func (wb *withBackoff) apply(config *retryConfig) {
config.backoff = &wb.backoff
}

// WithMaxAttempts configures the maximum number of times an API call can be made
// in the case of retryable errors.
// For example, if you set WithMaxAttempts(5), the operation will be attempted up to 5
// times total (initial call plus 4 retries).
// Without this setting, operations will continue retrying indefinitely
// until either the context is canceled or a deadline is reached.
func WithMaxAttempts(maxAttempts int) RetryOption {
return &withMaxAttempts{
maxAttempts: maxAttempts,
}
}

type withMaxAttempts struct {
maxAttempts int
}

func (wb *withMaxAttempts) apply(config *retryConfig) {
config.maxAttempts = &wb.maxAttempts
}
Tulsishah marked this conversation as resolved.
Show resolved Hide resolved

// RetryPolicy describes the available policies for which operations should be
// retried. The default is `RetryIdempotent`.
type RetryPolicy int
Expand Down Expand Up @@ -2148,6 +2168,7 @@ type retryConfig struct {
backoff *gax.Backoff
policy RetryPolicy
shouldRetry func(err error) bool
maxAttempts *int
}

func (r *retryConfig) clone() *retryConfig {
Expand All @@ -2168,6 +2189,7 @@ func (r *retryConfig) clone() *retryConfig {
backoff: bo,
policy: r.policy,
shouldRetry: r.shouldRetry,
maxAttempts: r.maxAttempts,
}
}

Expand Down
40 changes: 40 additions & 0 deletions storage/storage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -953,6 +953,10 @@ func TestConditionErrors(t *testing.T) {
}
}

func expectedAttempts(value int) *int {
return &value
}

// Test that ObjectHandle.Retryer correctly configures the retry configuration
// in the ObjectHandle.
func TestObjectRetryer(t *testing.T) {
Expand All @@ -977,6 +981,7 @@ func TestObjectRetryer(t *testing.T) {
Max: 30 * time.Second,
Multiplier: 3,
}),
WithMaxAttempts(5),
WithPolicy(RetryAlways),
WithErrorFunc(func(err error) bool { return false }))
},
Expand All @@ -986,6 +991,7 @@ func TestObjectRetryer(t *testing.T) {
Max: 30 * time.Second,
Multiplier: 3,
},
maxAttempts: expectedAttempts(5),
policy: RetryAlways,
shouldRetry: func(err error) bool { return false },
},
Expand All @@ -1012,6 +1018,15 @@ func TestObjectRetryer(t *testing.T) {
policy: RetryNever,
},
},
{
name: "set max retry attempts only",
call: func(o *ObjectHandle) *ObjectHandle {
return o.Retryer(WithMaxAttempts(11))
},
want: &retryConfig{
maxAttempts: expectedAttempts(11),
},
},
{
name: "set ErrorFunc only",
call: func(o *ObjectHandle) *ObjectHandle {
Expand Down Expand Up @@ -1063,6 +1078,7 @@ func TestClientSetRetry(t *testing.T) {
Max: 30 * time.Second,
Multiplier: 3,
}),
WithMaxAttempts(5),
WithPolicy(RetryAlways),
WithErrorFunc(func(err error) bool { return false }),
},
Expand All @@ -1072,6 +1088,7 @@ func TestClientSetRetry(t *testing.T) {
Max: 30 * time.Second,
Multiplier: 3,
},
maxAttempts: expectedAttempts(5),
policy: RetryAlways,
shouldRetry: func(err error) bool { return false },
},
Expand All @@ -1097,6 +1114,15 @@ func TestClientSetRetry(t *testing.T) {
policy: RetryNever,
},
},
{
name: "set max retry attempts only",
clientOptions: []RetryOption{
WithMaxAttempts(7),
},
want: &retryConfig{
maxAttempts: expectedAttempts(7),
},
},
{
name: "set ErrorFunc only",
clientOptions: []RetryOption{
Expand Down Expand Up @@ -1150,10 +1176,12 @@ func TestRetryer(t *testing.T) {
name: "object retryer configures retry",
objectOptions: []RetryOption{
WithPolicy(RetryAlways),
WithMaxAttempts(5),
WithErrorFunc(ShouldRetry),
},
want: &retryConfig{
shouldRetry: ShouldRetry,
maxAttempts: expectedAttempts(5),
policy: RetryAlways,
},
},
Expand All @@ -1166,6 +1194,7 @@ func TestRetryer(t *testing.T) {
Multiplier: 6,
}),
WithPolicy(RetryAlways),
WithMaxAttempts(11),
WithErrorFunc(ShouldRetry),
},
want: &retryConfig{
Expand All @@ -1175,6 +1204,7 @@ func TestRetryer(t *testing.T) {
Multiplier: 6,
},
shouldRetry: ShouldRetry,
maxAttempts: expectedAttempts(11),
policy: RetryAlways,
},
},
Expand All @@ -1187,6 +1217,7 @@ func TestRetryer(t *testing.T) {
Multiplier: 6,
}),
WithPolicy(RetryAlways),
WithMaxAttempts(7),
WithErrorFunc(ShouldRetry),
},
want: &retryConfig{
Expand All @@ -1196,6 +1227,7 @@ func TestRetryer(t *testing.T) {
Multiplier: 6,
},
shouldRetry: ShouldRetry,
maxAttempts: expectedAttempts(7),
policy: RetryAlways,
},
},
Expand All @@ -1206,10 +1238,12 @@ func TestRetryer(t *testing.T) {
},
objectOptions: []RetryOption{
WithPolicy(RetryNever),
WithMaxAttempts(5),
WithErrorFunc(ShouldRetry),
},
want: &retryConfig{
policy: RetryNever,
maxAttempts: expectedAttempts(5),
shouldRetry: ShouldRetry,
},
},
Expand All @@ -1220,10 +1254,12 @@ func TestRetryer(t *testing.T) {
},
objectOptions: []RetryOption{
WithPolicy(RetryNever),
WithMaxAttempts(11),
WithErrorFunc(ShouldRetry),
},
want: &retryConfig{
policy: RetryNever,
maxAttempts: expectedAttempts(11),
shouldRetry: ShouldRetry,
},
},
Expand All @@ -1243,9 +1279,11 @@ func TestRetryer(t *testing.T) {
Max: time.Microsecond,
}),
WithErrorFunc(ShouldRetry),
WithMaxAttempts(5),
},
want: &retryConfig{
policy: RetryAlways,
maxAttempts: expectedAttempts(5),
shouldRetry: ShouldRetry,
backoff: &gax.Backoff{
Initial: time.Nanosecond,
Expand Down Expand Up @@ -1280,6 +1318,7 @@ func TestRetryer(t *testing.T) {
bucketOptions: []RetryOption{
WithPolicy(RetryNever),
WithErrorFunc(ShouldRetry),
WithMaxAttempts(5),
},
objectOptions: []RetryOption{
WithBackoff(gax.Backoff{
Expand All @@ -1289,6 +1328,7 @@ func TestRetryer(t *testing.T) {
},
want: &retryConfig{
policy: RetryNever,
maxAttempts: expectedAttempts(5),
shouldRetry: ShouldRetry,
backoff: &gax.Backoff{
Initial: time.Nanosecond,
Expand Down