Swallow (and attempt to retry) Redis timeout errors when updating stats.... #1511

Merged
merged 6 commits into from Feb 25, 2014

Projects

None yet

2 participants

@jonhyman
Collaborator

... This is so code which calls #stats does not conflate job failures with stats failures.

@jonhyman jonhyman Swallow (and attempt to retry) Redis timeout errors when updating sta…
…ts. This is so code which calls #stats does not conflate job failures with stats failures.
0a58b7f
@mperham
Owner

Shouldn't we swallow anything generated by the redis block, not just TimeoutError?

@jonhyman
Collaborator

It does. It just retries for TimeoutErrors.

@jonhyman
Collaborator

In my experience of seeing this, I've only seen Redis::TimeoutErrors which I know are safe to retry. If something else is happening I don't know if it's safe to retry it (probably is but just to be safe)

@mperham
Owner

Oh right, and we shouldn't retry generic network exceptions?

@jonhyman
Collaborator

That's true, we should retry those things like Errno::ECONNRESET and such. I'll just have it retry everything.

@mperham
Owner

I'd suggest a sleep(1) in there for retries too. Should handle the case of restarting Redis while Sidekiq is actively working.

@mperham

StandardError is superfluous.

@jonhyman
Collaborator

Updated.

@mperham
Owner

Changelog?

@jonhyman
Collaborator

Added.

@mperham mperham merged commit 48446bb into master Feb 25, 2014

1 check passed

Details default The Travis CI build passed
@jonhyman
Collaborator

Many thanks for a swift review.

@mperham mperham deleted the feature/redis-stats-protection branch Jul 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment