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

pass the exception to failure hook #721

Closed

Conversation

Nowaker
Copy link

@Nowaker Nowaker commented Oct 7, 2014

I use delayed_job to run background tasks in VirtKick, example tasks are creating a new machine, creating a new disk, mounting an ISO, etc. Some I/O related tasks can't be performed when load is high, libvirt is just rejecting the call. Therefore, I set some big max_attempts and reschedule_at in 2 seconds. That's fine.

module Bugsnagable
  def error job, e
    unless Rails.env.test?
      puts e.message
      puts e.backtrace.map { |e| '    ' + e }.join "\n"
    end
    Bugsnag.notify_or_ignore e
  end
end

I catch every unhandled exception with Bugsnag so as to know of every exception. The problem is this reports too much. If the job fails for the first time and succeeds for the second, one exception will be reported anyway. One way would be to check if attempt == max_attempts before reporting to Bugsnag, but I believe the best way is to introduce def failure(job, exception).

I didn't write any specs at this point - tell me if you're OK with def failure job, exception first. :) Please also see my questions directly in the code.

@@ -211,7 +211,7 @@ def run(job)

# Reschedule the job in the future (when a job fails).
# Uses an exponential scale depending on the number of failed attempts.
def reschedule(job, time = nil)
def reschedule(job, time = nil, error = nil)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

time param is never used across the code. All tests still pass when removed. Can we get rid of it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically part of the public API. Leave it.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling d913e92 on Nowaker:feature/exception-in-failure-hook into c85a4fe on collectiveidea:master.

@sferik sferik force-pushed the master branch 2 times, most recently from 00c7e65 to 27ce6b0 Compare October 8, 2014 16:38
@albus522
Copy link
Member

albus522 commented Oct 9, 2014

This needs to work with old failure callbacks that only accept 1 argument. This PR would break everyone's current code.

@Nowaker
Copy link
Author

Nowaker commented Oct 9, 2014

@albus522 There haven't been any specs for error hooks, right? Will write and make sure all works OK.

Please also answer my question in the code.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling d1a9cde on Nowaker:feature/exception-in-failure-hook into 6d0a6b2 on collectiveidea:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling f13e6af on Nowaker:feature/exception-in-failure-hook into 6d0a6b2 on collectiveidea:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling db016ee on Nowaker:feature/exception-in-failure-hook into 6d0a6b2 on collectiveidea:master.

@Nowaker
Copy link
Author

Nowaker commented Dec 21, 2014

Will submit a new PR when done.

@Nowaker Nowaker closed this Dec 21, 2014
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.

3 participants