extend standard retry with 429 http status code #2223
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request does not have a backport label. Could you fix it @orouz? 🙏
|
📊 Allure Report - 💚 No failures were reported.
|
orouz
commented
May 23, 2024
Comment on lines
+34
to
+41
| awsConfig.Retryer = func() aws.Retryer { | ||
| return retry.NewStandard(func(o *retry.StandardOptions) { | ||
| o.Retryables = append(o.Retryables, retry.RetryableHTTPStatusCode{ | ||
| Codes: map[int]struct{}{ | ||
| http.StatusTooManyRequests: {}, | ||
| }, | ||
| }) | ||
| }) |
There was a problem hiding this comment.
this is essentialy the same as some AWS client does manually
orouz
commented
May 23, 2024
| } | ||
| awsConfig.Region = metadata.Region |
There was a problem hiding this comment.
all our AWS config initializations are done via libbeat. the only extra thing the config provider did was set the region. it was only used by EKS, which now sets the region itself
orouz
commented
May 23, 2024
| awsIdentityProvider: mockAwsIdentityProvider(errors.New("some error")), | ||
| clientProvider: mockKubeClient(nil), | ||
| awsMetadataProvider: mockMetadataProvider(errors.New("not this error")), // ignored | ||
| awsMetadataProvider: mockMetadataProvider(nil), |
There was a problem hiding this comment.
this needs to not err because EKS now sets the region itself via the metadata provider
moukoublen
approved these changes
May 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of your changes
this PR:
InitializeAWSConfigfunction ends up returning anaws.ConfigwithRetryerasnil, so without adding it we're not opting-in to the default retry mechanism429as an additional, retryable HTTP status code.Implementation notes
the first commit of this PR can be considered an alternative, standalone implementation of doing the same thing the 2nd commit does. the difference between the two, is that the 2nd commit removes the
ConfigProviderin favor of a plain function to encapsulate AWS config initialization (via libbeat) + adding a retryer. to do so, some code was removed and now all AWS config initializations call the same function, and the only consumer of the now-deleted AWS config provider (EKS) sets the region manually (which is what the provider did)all in all - the 2nd commit's diff is bigger, but i think it does some nice cleanup too. although, we could go with the 1st if preferable.
Related Issues
Checklist