quintilesims / layer0

Build, Manage, and Deploy Your Applications
Apache License 2.0
44 stars 20 forks source link

423: Implement retry and delay logic against AWS API #428

Closed jparsons04 closed 6 years ago

jparsons04 commented 6 years ago

What does this pull request do? In reference to issue #423. This adds retry and delay configuration options to help with rate limiting issues that have previously been encountered.

~This introduces three new API flags: a min retry, a max retry and a time between requests flag.~

~We are using a Config.Retryer which is a RequestRetryer (an interface) which is in turn an alias for a type that implements the request.Retryer interface.~

~This interface has three methods and I've opted to implement two of these methods at this time.~

How should this be tested?

~There might be a way to test this by unit testing the custom methods of the Retryer. Other suggestions for testing this would be welcome.~

This is just adding some custom configuration flags and environment variables now, so this could be tested by setting these configuration values by hand and observing results.

Checklist ~- [ ] Unit tests~ ~- [ ] Smoke tests (if applicable)~ ~- [ ] System tests (if applicable)~ ~- [ ] Documentation (if applicable)~

closes #423

jparsons04 commented 6 years ago

One thing I could certainly do in the interim to make this code cleaner would be to properly implement the Retryer interface rather than short circuit it as I've done here so far.

jparsons04 commented 6 years ago

@zpatrick @diemonster I removed the custom retryer stuff as requested and also made a envvar/flag for customizing the max number of retries.

jparsons04 commented 6 years ago

I removed the requirement for unit tests as we are not implementing a custom retryer. This now just essentially adds a couple of flags/envvars to customize and enforce the time between AWS requests and the max number of retries.

diemonster commented 6 years ago

@jparsons04 have you done any of the testing by hand that you mentioned in the PR details?

jparsons04 commented 6 years ago

Yes, I have.

As one example, I set up an environment using terraform and set the environment variable before destroying it, then destroyed via terraform. I inserted some debug statements when the PushBack method was being invoked:

With a delay: https://gist.github.com/jparsons04/1d5c0323a4b9efd9987da1c2d8c62efb Without a delay: https://gist.github.com/jparsons04/b2d1eed7f29b10c5812e2216b2077492

You can see a significant delay between requests when I define the environment variable. Though there are sometimes when two requests are made in rapid succession. Nonetheless, the vast majority of the requests have delays between them.