BUGFIX: Remove from InProgress if we don't addToDead #92
Conversation
…hed regardless of the outcome; otherwise locks are retained for the lifetime of the worker. The easiest way to ensure it's called in every branch is to remove it from branches entirely. Its behavior can be modified selectively by passing in a closure, which can perform redis operations inside the same transaction as the common housekeeping ops.
|
I just realized I never ran before / after benchmarks, so caveat emptor. I plan to do that this morning. |
|
Not exactly statistically rigorous, but doesn't seem to be much difference (mine -> built with this PR, theirs -> built with 85f9368): https://gist.github.com/davars/f70ed4f9f2b2c90429d0b94054c55afa |
|
I plan to spend some time reviewing this today |
worker.go
Outdated
|
|
||
| func (w *worker) addToRetry(job *Job, runErr error) { | ||
| func terminateOnly(_ redis.Conn) { return } | ||
| func terminateAndRetry(w *worker, jt *jobType, job *Job) terminateOp { | ||
| rawJSON, err := job.serialize() | ||
| if err != nil { | ||
| logError("worker.add_to_retry", err) |
There was a problem hiding this comment.
Plz change the error key to worker.terminate_and_retry.serialize.
worker.go
Outdated
| rawJSON, err := job.serialize() | ||
|
|
||
| if err != nil { | ||
| logError("worker.add_to_dead.serialize", err) |
There was a problem hiding this comment.
Plz change error key to worker.terminate_and_dead.serialize.
| @@ -39,6 +40,13 @@ type jobType struct { | |||
| DynamicHandler reflect.Value | |||
| } | |||
|
|
|||
| func (jt *jobType) calcBackoff(j *Job) int64 { | |||
There was a problem hiding this comment.
Nice! thanks for adding this.
|
So @davars was the issue that when we exhausted retries the job and the job was marked It looks like the redis commands used by Aside from this being a nice refactor, I'm guessing you tracked down a specific path that lead to dead jobs not being cleaned up properly. |
|
You're correct. In my setup I have my cron jobs enqueued with Working on the error messages now. Side note, there's a rare case where locks get stuck and aren't cleaned up by the reaper. I haven't tracked that one down yet. I suspect it may be due to concurrent reapers, since it looks like each pool runs reapers without coordination. Maybe the reaper task should be an internal job that's run by the scheduler. Then it would get a job id, lock, be run by one pool per period, etc. |
Tracked down why locks were being retained by jobs that weren't running to this branch. First commit is a simple fix. Second commit is a bit more involved and pulls removeJobFromInProgress out of any branches to ensure it always runs.
Commit Messages:
Remove from InProgress if we don't addToDead
removeJobFromInProgress critically needs to run when a job is dispatched regardless of the outcome; otherwise locks are retained for the lifetime of the worker. The easiest way to ensure it's called in every branch is to remove it from branches entirely. Its behavior can be modified selectively by passing in a closure, which can perform redis operations inside the same transaction as the common housekeeping ops.
Consistent error messages