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

provider/aws: Beanstalk environments, bump the minimum timeout between API calls #7523

Merged
merged 6 commits into from
Jul 21, 2016

Conversation

catsby
Copy link
Contributor

@catsby catsby commented Jul 6, 2016

This is a band-aid to #6870. Beanstalk environments can take a long time to setup, and if you are managing many of them you can hit API rate limits for your account.

A more permanent solution may be to expose a knob to set a customizable timeout / poll interval, but as I mention in #6870 the design of such a knob hasn't been planed out yet.

@@ -249,7 +249,7 @@ func resourceAwsElasticBeanstalkEnvironmentCreate(d *schema.ResourceData, meta i
Refresh: environmentStateRefreshFunc(conn, d.Id()),
Timeout: waitForReadyTimeOut,
Delay: 10 * time.Second,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it the delay you want to update?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delay is the amount of time before we start. MinTimeout is the minimum amount of time between refreshes:

Delay          time.Duration    // Wait this time before starting checks
MinTimeout     time.Duration    // Smallest time to wait before refreshes

I'll be refactoring this PR to expose at least MinTimeout, stay tuned!

@catsby catsby force-pushed the f-aws-beanstalk-env-poll-timing branch from 3875d54 to d2c9cc3 Compare July 8, 2016 17:19
}
if duration < 10*time.Second || duration > 60*time.Second {
errors = append(errors, fmt.Errorf(
"%q must be between 10s and 60s", k))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10s is our current minimum. 60s seemed like a reasonable max, but we can tune it higher if need be

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

60s seems like a reasonable max, I'd agree. You don't want atlas jobs taking forever because of someone's value here. Its a fine line.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I like poll_interval as the variable, seems to be generic that could be added in another providers / resources in the future.

@phinze
Copy link
Contributor

phinze commented Jul 8, 2016

I'm not sure if MinTimeout does what we want here:

wait := time.Duration(math.Pow(2, float64(tries))) *
100 * time.Millisecond
if wait < conf.MinTimeout {
wait = conf.MinTimeout
} else if wait > 10*time.Second {
wait = 10 * time.Second
}

Looks like it's capped at 10s based on that conditional.

That's all based on doing exponential backoff. Maybe we need to plumb through a fresh option for a static poll interval if the user requests it?

@catsby
Copy link
Contributor Author

catsby commented Jul 8, 2016

Paul and I discussed this and found that MinTimeout was in fact "working" as I expected it, however, we discovered that it would only do so for about 10 minutes, until the calculated back off time exceeded the 60s, in which case the polling would return to every 10 seconds 😵

I'm going to take another pass and try adding a separate config option that's preferable to the min timeout

@catsby catsby force-pushed the f-aws-beanstalk-env-poll-timing branch from 46b4d51 to d74c2d5 Compare July 13, 2016 21:59
@catsby
Copy link
Contributor Author

catsby commented Jul 13, 2016

@phinze I refactored this to use poll_interval as a separate thing in WaitForState.

@phinze
Copy link
Contributor

phinze commented Jul 21, 2016

LGTM

@ghost
Copy link

ghost commented Apr 24, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants