-
Notifications
You must be signed in to change notification settings - Fork 76
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 support for retryable errors through Qless::RetryExceptions mixin. #63
Conversation
I'd like to see an integration test where the error is a retryable exception but the job has exhausted it's retries. |
job.retry | ||
else | ||
fail_job(job, error) | ||
end | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it's not normal to do so, you can use a ruby expression in a rescue clause. Given that, I think you can simplify things a lot by doing something like:
def retryable_exceptions
return [] unless job.klass.respond_to?(:retryable_exceptions)
job.klass.retryable_exceptions
end
def perform(job)
around_perform(job)
rescue *retryable_exceptions
job.retry
rescue Exception => error
fail_job(job, error)
end
You also wouldn't need to define retryable_exception?
(with it's any?
/is_a?
logic)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, a side benefit of this: ruby allows anything to be used in a rescue clause that responds to ===
and uses that to see if the exception matches. The normal thing is to use class constants, but it gives you the power to use an object that inspects the message or whatever. By leveraging ruby's built-in logic, it allows things to be more flexible here and work the same way as ruby itself does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I was confused about raise
vs rescue
requiring a subclass of Exception
and failing if anything else is provided. The reason I went with a retryable_exception?
method is that it allows for the same flexibility you're describing with ===
. I'll make the code change.
Add support for retryable errors through Qless::RetryExceptions mixin.
Was it intentional that job classes that don't use this mixin won't be retried at all? class MyJob
def self.perform(job)
raise 'hell'
end
end This job fails immediately, without any retries. It seems that the old behaviour (retry on any exception) should be preserved for job classes that opt out of the explicit declaration. Thoughts? |
@jstorimer -- before this change, jobs were never retried automatically by the worker. If you wanted to retry a job, you were responsible for calling |
Ohhh I see. I'm just getting back to qless after not touching it for a few months. I hadn't realized that was the previous behaviour. Forget I said anything 🚶 Now I appreciate the fact that I can do |
@jstorimer -- No worries :). I appreciate having other folks that are using Qless commenting on this stuff, so thanks! On a side note, I'm curious as to how you're using Qless (and I imagine @dlecocq is, too). We're using it on quite a few projects internally at SEOmoz, but hearing how other folks are using Qless (potentially in entirely different ways) will help inform how we change Qless over time. If you feel up for it, can you write up something explaining how you're using it? (e.g. which Qless features do you rely on heavily? How do you structure your job pipelines? Is it performing adequately for you?) Feel free to write up your response here or send an email to @dlecocq and I. |
Closes #29.