Skip to content

Commit

Permalink
fixing comment
Browse files Browse the repository at this point in the history
  • Loading branch information
@Tulsishah
Tulsishah committed Jan 6, 2024
1 parent f3ac62f commit 17890c9
Show file tree
Hide file tree
Showing 5 changed files with 73 additions and 61 deletions.
8 changes: 4 additions & 4 deletions storage/bucket_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1080,9 +1080,9 @@ func TestBucketRetryer(t *testing.T) {
Max: 30 * time.Second,
Multiplier: 3,
},
policy: RetryAlways,
maxRetryCount: 5,
shouldRetry: func(err error) bool { return false },
policy: RetryAlways,
maxAttempts: 5,
shouldRetry: func(err error) bool { return false },
},
},
{
Expand Down Expand Up @@ -1113,7 +1113,7 @@ func TestBucketRetryer(t *testing.T) {
return b.Retryer(WithMaxAttempts(5))
},
want: &retryConfig{
maxRetryCount: 5,
maxAttempts: 5,
},
},
{
Expand Down
4 changes: 2 additions & 2 deletions storage/invoke.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,10 @@ 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)
attempts++
if attempts == retry.maxRetryCount {
if retry.maxAttempts != 0 && attempts > retry.maxAttempts {
return true, err
}
attempts++
return !errorFunc(err), err
})
}
Expand Down
30 changes: 20 additions & 10 deletions storage/invoke_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,25 +172,25 @@ func TestInvoke(t *testing.T) {
expectFinalErr: false,
},
{
desc: "non-idempotent retriable error retried when policy is RetryAlways till max-retry-count",
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, maxRetryCount: 2},
retry: &retryConfig{policy: RetryAlways, maxAttempts: 2},
expectFinalErr: false,
},
{
desc: "non-idempotent retriable error retried when policy is RetryNever with max-retry-count set",
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, maxRetryCount: 2},
retry: &retryConfig{policy: RetryNever, maxAttempts: 2},
expectFinalErr: false,
},
{
desc: "non-retriable error retried with custom fn till max-retry-count",
desc: "non-retriable error retried with custom fn till maxAttempts",
count: 4,
initialErr: io.ErrNoProgress,
finalErr: nil,
Expand All @@ -199,10 +199,19 @@ func TestInvoke(t *testing.T) {
shouldRetry: func(err error) bool {
return err == io.ErrNoProgress
},
maxRetryCount: 2,
maxAttempts: 2,
},
expectFinalErr: false,
},
{
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: false,
retry: &retryConfig{policy: RetryAlways, maxAttempts: 4},
expectFinalErr: true,
},
} {
t.Run(test.desc, func(s *testing.T) {
counter := 0
Expand All @@ -218,7 +227,7 @@ func TestInvoke(t *testing.T) {
headers := callctx.HeadersFromContext(ctx)
gotClientHeader = headers["x-goog-api-client"][0]
gotIdempotencyHeader = headers["x-goog-gcs-idempotency-token"][0]
if (counter <= test.count) || (test.retry != nil && counter == test.retry.maxRetryCount) {
if counter <= test.count {
return test.initialErr
}
return test.finalErr
Expand All @@ -230,12 +239,13 @@ func TestInvoke(t *testing.T) {
s.Errorf("got %v, want %v", got, test.initialErr)
}
wantAttempts := 1 + test.count
if test.retry != nil && test.retry.maxRetryCount != 0 {
wantAttempts = test.retry.maxRetryCount - 1
}
if !test.expectFinalErr {
wantAttempts = 1
}
if test.retry != nil && test.retry.maxAttempts != 0 && test.retry.policy != RetryNever {
wantAttempts = 1 + 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
34 changes: 18 additions & 16 deletions storage/storage.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2076,19 +2076,21 @@ func (wb *withBackoff) apply(config *retryConfig) {
config.backoff = &wb.backoff
}

// WithMaxAttempts sets the maximum number of retries for operations that may fail.
func WithMaxAttempts(maxRetryCount int) RetryOption {
return &withMaxRetry{
maxRetryCount: maxRetryCount,
// Configures a maximum number of retries for potentially failing operations.

Check failure on line 2079 in storage/storage.go

View workflow job for this annotation

GitHub Actions / vet 

comment on exported function WithMaxAttempts should be of the form "WithMaxAttempts ..."
// 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 withMaxRetry struct {
maxRetryCount int
type withMaxAttempts struct {
maxAttempts int
}

func (wb *withMaxRetry) apply(config *retryConfig) {
config.maxRetryCount = wb.maxRetryCount
func (wb *withMaxAttempts) apply(config *retryConfig) {
config.maxAttempts = wb.maxAttempts
}

// RetryPolicy describes the available policies for which operations should be
Expand Down Expand Up @@ -2160,10 +2162,10 @@ func (wef *withErrorFunc) apply(config *retryConfig) {
}

type retryConfig struct {
backoff *gax.Backoff
policy RetryPolicy
shouldRetry func(err error) bool
maxRetryCount int
backoff *gax.Backoff
policy RetryPolicy
shouldRetry func(err error) bool
maxAttempts int
}

func (r *retryConfig) clone() *retryConfig {
Expand All @@ -2181,10 +2183,10 @@ func (r *retryConfig) clone() *retryConfig {
}

return &retryConfig{
backoff: bo,
policy: r.policy,
shouldRetry: r.shouldRetry,
maxRetryCount: r.maxRetryCount,
backoff: bo,
policy: r.policy,
shouldRetry: r.shouldRetry,
maxAttempts: r.maxAttempts,
}
}

Expand Down
58 changes: 29 additions & 29 deletions storage/storage_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -987,9 +987,9 @@ func TestObjectRetryer(t *testing.T) {
Max: 30 * time.Second,
Multiplier: 3,
},
maxRetryCount: 5,
policy: RetryAlways,
shouldRetry: func(err error) bool { return false },
maxAttempts: 5,
policy: RetryAlways,
shouldRetry: func(err error) bool { return false },
},
},
{
Expand Down Expand Up @@ -1020,7 +1020,7 @@ func TestObjectRetryer(t *testing.T) {
return o.Retryer(WithMaxAttempts(11))
},
want: &retryConfig{
maxRetryCount: 11,
maxAttempts: 11,
},
},
{
Expand Down Expand Up @@ -1084,9 +1084,9 @@ func TestClientSetRetry(t *testing.T) {
Max: 30 * time.Second,
Multiplier: 3,
},
maxRetryCount: 5,
policy: RetryAlways,
shouldRetry: func(err error) bool { return false },
maxAttempts: 5,
policy: RetryAlways,
shouldRetry: func(err error) bool { return false },
},
},
{
Expand Down Expand Up @@ -1116,7 +1116,7 @@ func TestClientSetRetry(t *testing.T) {
WithMaxAttempts(7),
},
want: &retryConfig{
maxRetryCount: 7,
maxAttempts: 7,
},
},
{
Expand Down Expand Up @@ -1176,9 +1176,9 @@ func TestRetryer(t *testing.T) {
WithErrorFunc(ShouldRetry),
},
want: &retryConfig{
shouldRetry: ShouldRetry,
maxRetryCount: 5,
policy: RetryAlways,
shouldRetry: ShouldRetry,
maxAttempts: 5,
policy: RetryAlways,
},
},
{
Expand All @@ -1199,9 +1199,9 @@ func TestRetryer(t *testing.T) {
Max: time.Hour,
Multiplier: 6,
},
shouldRetry: ShouldRetry,
maxRetryCount: 11,
policy: RetryAlways,
shouldRetry: ShouldRetry,
maxAttempts: 11,
policy: RetryAlways,
},
},
{
Expand All @@ -1222,9 +1222,9 @@ func TestRetryer(t *testing.T) {
Max: time.Hour,
Multiplier: 6,
},
shouldRetry: ShouldRetry,
maxRetryCount: 7,
policy: RetryAlways,
shouldRetry: ShouldRetry,
maxAttempts: 7,
policy: RetryAlways,
},
},
{
Expand All @@ -1238,9 +1238,9 @@ func TestRetryer(t *testing.T) {
WithErrorFunc(ShouldRetry),
},
want: &retryConfig{
policy: RetryNever,
maxRetryCount: 5,
shouldRetry: ShouldRetry,
policy: RetryNever,
maxAttempts: 5,
shouldRetry: ShouldRetry,
},
},
{
Expand All @@ -1254,9 +1254,9 @@ func TestRetryer(t *testing.T) {
WithErrorFunc(ShouldRetry),
},
want: &retryConfig{
policy: RetryNever,
maxRetryCount: 11,
shouldRetry: ShouldRetry,
policy: RetryNever,
maxAttempts: 11,
shouldRetry: ShouldRetry,
},
},
{
Expand All @@ -1278,9 +1278,9 @@ func TestRetryer(t *testing.T) {
WithMaxAttempts(5),
},
want: &retryConfig{
policy: RetryAlways,
maxRetryCount: 5,
shouldRetry: ShouldRetry,
policy: RetryAlways,
maxAttempts: 5,
shouldRetry: ShouldRetry,
backoff: &gax.Backoff{
Initial: time.Nanosecond,
Max: time.Microsecond,
Expand Down Expand Up @@ -1323,9 +1323,9 @@ func TestRetryer(t *testing.T) {
}),
},
want: &retryConfig{
policy: RetryNever,
maxRetryCount: 5,
shouldRetry: ShouldRetry,
policy: RetryNever,
maxAttempts: 5,
shouldRetry: ShouldRetry,
backoff: &gax.Backoff{
Initial: time.Nanosecond,
Max: time.Second,
Expand Down

0 comments on commit 17890c9

Please sign in to comment.