Auto retry fetch errors #429
Conversation
phstc
force-pushed
the
feature/428-auto-retry-fetch-failures
branch
from
bc57a4a
to
a3c9874
Compare
lib/shoryuken/fetcher.rb
Outdated
| sleep(attempts) | ||
|
|
||
| retry | ||
| end |
There was a problem hiding this comment.
@ethangunderson WDYT?
lib/shoryuken/fetcher.rb
Outdated
| @@ -9,20 +9,40 @@ def initialize(group) | |||
| end | |||
|
|
|||
| def fetch(queue, limit) | |||
| started_at = Time.now | |||
| do_with_retry(10) do | |||
There was a problem hiding this comment.
Any thoughts on making this configurable?
There was a problem hiding this comment.
@ethangunderson I usually make that stuff configurable on demand basis, if someone asks to configure, just to avoid adding extra options.
lib/shoryuken/fetcher.rb
Outdated
|
|
||
| logger.debug { "Retrying fetch attempt #{attempts} for #{ex.message}" } | ||
|
|
||
| sleep(attempts) |
There was a problem hiding this comment.
This will act as a backoff until max attempts is reached, right? I like that.
There was a problem hiding this comment.
@ethangunderson right!
|
Should this rescue from everything… or just fetch-related exceptions? |
|
@waynerobinson it used to rescue everything, then nothing, now it's rescuing everything again, but up to 3 attempts. I tried to find fetch-related exceptions before rescuing all, but it's hard to tell what are they. Is this broad rescue causing you issue? |
|
Nothing in particular. But in the past I've had issues catching broad exceptions and things like catching TERM signals. Would catching all StandardErrors be enough? It's pretty broad, but doesn't include any of the system level exceptions. |
|
@waynerobinson makes sense, but I believe
|
|
Ahh… fair enough then! 😀 |
|
@waynerobinson thanks a lot for review the changes 🍻 |
Fixes #428