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

aws/request: Reorganize retry behavior to reduce separation #2744

Merged
merged 2 commits into from
Aug 13, 2019

Conversation

jasdel
Copy link
Contributor

@jasdel jasdel commented Aug 9, 2019

Reorganizes the SDK's retry utilities to follow a consistent code path.
Request.IsErrorRetryable is the primary entry pointer for determining if
a request error should be retryable instead of split between
Request.Send and DefaultRetryer calling IsErrorRetryable. This also
gives the implementation of the Retryer interface consistent control
over when a request will be retried.

Also adds support for request and service specific API error retry and
throttling to be enabled by specifying API error codes before a request
is sent. The "RetryErrorCodes" and "ThrottleErrorCodes" members were added to
request.Request struct. These are used by the Request's IsErrorRetryable
and IsErrorThrottle respectively to specify additional API error codes
that identify the failed request attempt as needing to be retried or
throttled. The DefaultRetryer uses both of these methods when
determining if a failed request attempt should be retried.

Related to #1376

TODO

  • Need tests for new members added.

Reorganizes the SDK's retry utilities to follow a consistent code path.
Request.IsErrorRetryable is the primary entry pointer for determining if
a request error should be retryable instead of split between
Request.Send and DefaultRetryer calling IsErrorRetryable. This also
gives the implementation of the Retryer interface consistent control
over when a request will be retried.

Also adds support for request and service specific API error retry and
throttling to be enabled by specifying API error codes before a request
is sent. The "RetryCodes" and "ThrottleCodes" members were added to
request.Request struct. These are used by the Request's IsErrorRetryable
and IsErrorThrottle respectively to specify additional API error codes
that identify the failed request attempt as needing to be retried or
throttled. The DefaultRetryer uses both of these methods when
determining if a failed request attempt should be retried.

Related to aws#1376
@jasdel jasdel requested a review from skotambkar August 9, 2019 21:59
@jasdel jasdel added the needs-review This issue or pull request needs review from a core team member. label Aug 9, 2019
@jasdel jasdel merged commit 2a80185 into aws:master Aug 13, 2019
@jasdel jasdel deleted the feat/00_SvcRetryErrs branch August 13, 2019 16:37
@thastings3
Copy link

@jasdel when we wrap the aws clients Publish(*sns.PublishInput) (*sns.PublishOutput, error) with some of our own backoff/jitter we are using the error returned and passing that into request.IsErrorRetryable(error) which is causing us to always get true back for what we consider successful cases of nil error due to the addition of the case nil: return true in this pr inside of shouldRetryError.

My question is - are we using this helper function incorrectly on our side?
For now I have just added a nil check into our wrapper so its not a big deal but I was just curious if there was something more here.
Thanks!

@jasdel
Copy link
Contributor Author

jasdel commented Aug 21, 2019

Thanks for reporting this issue @thastings3. I created #2773 to track this bug. Passing nil into IsErrorRetryable documents that passing in nil should return false, and I reproduced this, and have a fix out for this issue shortly.

skotambkar added a commit to aws/aws-sdk-go-v2 that referenced this pull request Sep 17, 2019
Ports v1 SDK's retry utilities to follow a consistent code path. aws.IsErrorRetryable is the primary entry point to determine if a request is retryable. Also ports support for service/kinesis to retry service specific API errors . Corrects sdk's behavior by not retrying errors with status code 501.

Related to aws/aws-sdk-go#2744, aws/aws-sdk-go#2774, aws/aws-sdk-go#2751 and aws/aws-sdk-go#1826

Fixes #372; Fixes #145
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-review This issue or pull request needs review from a core team member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants