que: [Proposal] Per-class error_handler

@chanks @joevandyk How would you feel about me sending a PR for per-class error_handler?

Spiked an implementation like (inside job.rb):

if job
  if klass && klass.respond_to?(:error_handler)
    klass.error_handler(error, job)
  else
    count    = job[:error_count].to_i + 1
    interval = klass && klass.respond_to?(:retry_interval) && klass.retry_interval || retry_interval
    delay    = interval.respond_to?(:call) ? interval.call(count) : interval
    message  = "#{error.message}\n#{error.backtrace.join("\n")}"
    Que.execute :set_error, [count, delay, message] + job.values_at(:queue, :priority, :run_at, :job_id)
  end
end

The only question I have is determining if the job should be destroyed or left up to the method to ensure that the job is destroyed. Seems risky to leave jobs hanging around.

I could add something so that Que.execute :set_error just sets the error and not a new run_at. And then change the current usage to be something like Que.execute :set_error_and_retry.

About this issue

Original URL
State: closed
Created 8 years ago
Comments: 18

Most upvoted comments

Alright, I just updated it. I had forgotten this, but it renames Que.error_handler to Que.error_notifier, since that’s more descriptive of what it actually does now.

The Job API is basically:

class MyJob < Que::Job
  def run(*args)
    Thing.do_stuff(*args)
  end

  private

  def handle_error(error)
    case error
    when ErrorType
      super # Default behavior, uses retry_interval.
    when OtherErrorType
      retry_in 60 # Saves error message and all, but sets retry to 60 seconds.
    else
      destroy # Don't retry at all.
    end
  end
end

Additionally, the return value of handle_error dictates whether the error is passed to the error_notifier. The superclass method and retry_in and destroy all return true, but you can suppress a notification by returning false from your handle_error method after you’ve called one of those.

Anyway, yeah, the API is certainly open for debate 😄

chanks on Apr 8, 2016