-
Notifications
You must be signed in to change notification settings - Fork 17
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
Retry GetResource NotFound when creating #1809
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1809 +/- ##
==========================================
+ Coverage 45.44% 45.67% +0.22%
==========================================
Files 38 38
Lines 5886 5916 +30
==========================================
+ Hits 2675 2702 +27
- Misses 2988 2991 +3
Partials 223 223 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ctx context.Context, | ||
typeName, identifier string, | ||
) (map[string]interface{}, error) { | ||
retryBackoff := retry.NewExponentialJitterBackoff(30 * time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd reduce the max duration here a bit. We'd now wait up to ~60 seconds. That seems a bit too long to me.
Eventual consistency delays for those services are usually in the millis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll send a quick follow-up PR, this one merged already.
GetResource NotFound backoff introduced in #1809 allowed long time wait periods up to 30 seconds. This change makes the max wait period 3 seconds at this is more appropriate for AWS eventual consistency propagation delays this backoff tries to compensate for.
Introduces a retry for NotFound errors from GetResource executed right after Create.
Based on the logs from our CI runs we suspect eventual consistency in AWS may cause some resources to fail with NotFound in Get even after WaitForResourceOpCompletion succeeded.
Relates: #1186