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

Add 1 to retry_count To Properly Report Exhausted Failures #121

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mstruve
Copy link

@mstruve mstruve commented Dec 3, 2018

I noticed errors were not being reported when I used the :exhausted option. Since the retry_count for Sidekiq starts at 0, if you have the retry count option manually set in your job it will never trigger an error to be reported when the retries are exhausted.

Let me know what you think!

@mcasper
Copy link
Collaborator

mcasper commented Jun 27, 2020

Hey @mstruve, thanks for the contribution! It looks like this is likely only a bug in sidekiq 5+, when they moved all internal middleware out of the middleware chain. In <5 we rely on the ordering of the middleware for this to work properly (https://github.com/mhfs/sidekiq-failures/blob/master/lib/sidekiq/failures.rb#L84), and in >=5 the retry logic is permanently after our logic.

That being said, since we still support Sidekiq 4 would you be willing to adjust the logic to account for the version differences, and add some tests for it?

@mstruve
Copy link
Author

mstruve commented Jun 27, 2020

Hey @mcasper! Thanks for getting back to me, I am not currently using this gem anymore so I don't think I will have time to work on this. Please feel free to close and if I need it in the future I will get back to it. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants