[Proposal] Per-class error_handler

[tpitale]

@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.

[chanks]

I made a branch a while ago (https://github.com/chanks/que/compare/error-handling) to support a handle_error method in classes. This feels better to me than the class-level configuration used so far in Que because it's more composable - you can implement whatever error logic you want in a module and then include it in whatever job classes you want it to apply to (or just include it in Que::Job directly, for that matter). You can see the API I was thinking of in this comment: #106 (comment)

I haven't put more work into it because nobody else seemed really invested in it and I don't have any projects using Que right now, so it hasn't been a priority for me (though I expect that to change in the next month or two 😄). But does that sort of API (where an instance method would be responsible for deciding whether to destroy or retry the job) seem reasonable and useful to you?

[tpitale]

That looks good! If you would rather go in that direction, I'm happy to close this.

[chanks]

I think it makes more sense? But I haven't actually tried it, and I think
that branch is out of date. Friday is my open source day at work, I can try
to look into it then. I'm kind of interested in other critiques before
settling on it, though - there was conversation about it in another issue
that stalled out, as I recall. But at least I can bring it up to date and
give you something to try out, if this is a problem you're needing to solve
soon.

On Friday, March 25, 2016, Tony Pitale notifications@github.com wrote:

That looks good! If you would rather go in that direction, I'm happy to
close this.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#147 (comment)

[tpitale]

Yeah, I'm not in a huge rush. I'd say bring the branch up-to-date, and open for discussion.

[chanks]

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 😄

[tpitale]

I think that looks like a great first pass. It will work perfectly for my needs. Would love to get a alpha/beta gem to try out!

[chanks]

Can you use bundler to point at the ref? I don't really want to put out an
API that people will start to rely on, even if it's explicitly marked as an
alpha or beta.

On Fri, Apr 8, 2016 at 10:46 PM, Tony Pitale notifications@github.com
wrote:

I think that looks like a great first pass. It will work perfectly for my
needs. Would love to get a alpha/beta gem to try out!

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#147 (comment)

[tpitale]

Oh, yeah, I'm already doing that.

[tpitale]

This has been working great for a few weeks now. Do you want to make it a PR and open for larger discussion @chanks?

[tpitale]

@chanks Any chance we could merge the error-handling branch and release a gem? This branch has been running great for months.

[chanks]

Yeah, I think it makes sense to merge this and #131. I'll find time on Friday to release a new version.

[tpitale]

Excellent, thanks!

[chanks]

It's merged, just need to find the time to write some docs. Blah.

[tpitale]

I can write up a docs PR for you to review/edit, if that helps.

[chanks]

Didn't want to wait any more, so I just cut release 0.12.0. A docs PR that I could tweak would be appreciated, or I can get to it some other time. Either way!