feat(request): optionally retry network failures #353
Conversation
|
This is a re-submission of #280 @kyleconroy, I rebased, added the exponential backoff, and am now only adding an idempotency key to post requests. A couple questions:
|
| @@ -232,6 +268,15 @@ def self.request_headers(api_key) | |||
| end | |||
| end | |||
|
|
|||
| # the build machines run ruby 1.8.7, and so do not have SecureRandom | |||
There was a problem hiding this comment.
We no longer support Ruby 1.8.7, so we can assume that SecureRandom is defined.
That leaves the interface. cc @russelldavis @brandur for thoughts. |
|
How about just |
|
@russelldavis are you proposing that we don't expose the other values? |
|
Oh, sorry, I was just thinking of the first value. How about:
|
| retry_count = retry_count + 1 | ||
| sleep sleep_time(retry_count) | ||
| response = execute_request_with_rescues(request_opts, api_base_url, retry_count) | ||
| if self.on_successful_retry |
There was a problem hiding this comment.
What is the use case for this callback?
There was a problem hiding this comment.
I wanted to know if we were ever actually hitting this thing, so I put some logging in that callback. That's how I know this code has actually caught errors for us. (A bit of an edge case, for sure. I wouldn't complain too much if you wanted to remove it)
|
Looks good to me generally! Honestly, related to #313, this would be much more cleanly implemented as a Faraday or Excon middleware, but given that we're not there yet, it makes sense not to block on that. One possible idea here is to just remove the options for |
| raise APIConnectionError.new(message + "\n\n(Network error: #{e.message})") | ||
| end | ||
|
|
||
| def self.should_retry?(e, retry_count) | ||
| return false unless self.max_retries_on_network_failure > retry_count |
There was a problem hiding this comment.
nit: this seems easier to follow if rewritten as:
return false if retry_count >= self.max_retries_on_network_failure
|
looking forward to seeing this PR land! |
|
Agreed, keeping the surface area of this change low (one settable attribute) seems like a smart choice. I'd also be a fan of getting rid of the retry callback. |
|
@kyleconroy, summarizing the discussion above, I'm going to do the following:
Anything else I missed? Thanks for the comments. |
Other than that, looks good! |
nbrustein
force-pushed
the
retry-network-failures
branch
2 times, most recently
from
15d818b
to
66ced30
Compare
|
Should be good to go now. |
|
@kyleconroy - anything else blocking this feature? |
|
@nbrustein |
nbrustein
force-pushed
the
retry-network-failures
branch
from
66ced30
to
a31986f
Compare
|
@kyleconroy those changes are in. Let me know if there's anything else. |
|
LGTM @brandur can you take one last look as well? |
|
@kyleconroy +1 from me. @nbrustein Thanks for the patch! |
feat(request): optionally retry network failures
|
Thanks again @nbrustein. We'll cut a new release in around a week, as we'll want to wait for the holidays to be over. |
|
👍 |
|
Hi @kyleconroy, could you please update rubygems so we can see see this change without specifying github repository in Gemfile? Thanks! |
|
@dziemian007 Released as 1.32.0. Sorry about the delay! |
|
🎉 |
Retries can be turned on by setting
Once the
max_retries_on_network_failureis set to a positive integer, and request that fails on a network error will be retried. There will be a sleep between each retry, the length of which is determined using an exponential backoff algorithm.If a post request is made without an idempotency key and retries are on, then an idempotency key will be added automatically, since it is unsafe to retry without one.
max_retry_sleep_secondsis a float that can be set to configure that maximum time to sleep between retries (default is 2 seconds)base_retry_sleep_secondsis a float can be set to configure the time to wait before the initial retry (default is 0.5 seconds)