-
Notifications
You must be signed in to change notification settings - Fork 618
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
separated logic for warm pools polling scenarios and do not fail on t… #3055
separated logic for warm pools polling scenarios and do not fail on t… #3055
Conversation
agent/app/agent.go
Outdated
return err | ||
} | ||
// Poll while the instance is in a warmed state until it is going to go into service | ||
for targetState != "InService" { |
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.
nit: can we use inServiceState
here?
// Poll while the instance is in a warmed state until it is going to go into service | ||
for targetState != "InService" { | ||
time.Sleep(pollWaitDuration) | ||
targetState, err = agent.getTargetLifecycle(maxRetries) |
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 think the retries make sense for the first stage (i.e. pollUntilTargetLifecyclePresent
), but what do you think of invoking this function like agent.getTargetLifecycle(1)
at this point, since this loop will retry anyways.
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.
Good point. The main advantage I would see is the situation where we might get an errors on the API call when the instance becomes ready, and then delay the set up by a number of minutes. That would be an edge case, though I think would not be a great experience if it were to occur, so I would be inclined to keep more than one retry.
That said, thinking about it, I don't think the increase in retry count is really necessary, it should be fine at 3. In the second stage, we will retry anyway, and in the first stage we wouldn't expect throttling errors to be likely and wouldn't need additional retries in the failure scenarios
…hrottling or transient server errors once state obtained
f079fbe
to
e9c65fa
Compare
…hrottling or transient server errors once state obtained (aws#3055) Co-authored-by: Lydia Filipe <fillydia@amazon.com>
…hrottling or transient server errors once state obtained (aws#3055) Co-authored-by: Lydia Filipe <fillydia@amazon.com>
Commit: separated logic for warm pools polling scenarios and do not fail on throttling or transient server errors once state obtained
Summary
Implementation details
waitUntilInstanceInService
method to continue polling for certain errors once target state obtained.pollUntilLifecycleStateObtained
for the initial polling for any value.Testing
Description for the changelog
Separated logic for warm pools polling scenarios and do not fail on throttling or transient server errors once state obtained
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.