feat: add job retrying and failed:fatal events - #227
Conversation
Sorry, something went wrong.
| @@ -545,9 +544,8 @@ class Queue extends Emitter { | |||
| } | |||
|
|
|||
| _finishJob(err, data, job) { | |||
There was a problem hiding this comment.
Refactored this method and moved some code to Job#computeDelay so we can compute the final status early.
Sorry, something went wrong.
| // Workaround for #184: emit failed event for backwards | ||
| // compatibility while affording for a separate event that | ||
| // identifies the final failure. | ||
| const emitExtra = | ||
| status === 'retrying' | ||
| ? 'failed' | ||
| : status === 'failed' | ||
| ? 'failed:fatal' | ||
| : null; | ||
| if (emitExtra) this.emit(emitExtra, job, result); |
There was a problem hiding this comment.
This is the key change that addresses #184.
Sorry, something went wrong.
|
Coverage remained the same at 100.0% when pulling f78ad0f on improve-job-events into 9d59aac on master. |
Sorry, something went wrong.
|
@skeggse this needs a rebase, are you able to take care of that? |
Sorry, something went wrong.
|
I could rebase, sure. That said, I would favor #186 over this pull request since you seem to be comfortable making breaking changes to bee-queue. Was the decision to close #186 in favor of #227 an explicit/intentional decision? What's the rationale, if I may ask? |
Sorry, something went wrong.
|
@skeggse actually I'm trying to avoid breaking changes, perhaps I misunderstood the effect of this PR? #186 was closed because it was a) apparently abandoned and b) you appeared to have problems with it. If #227 isn't the way to go, then we we should close it. |
Sorry, something went wrong.
|
Ah, fair enough then. I don't recall these discussions or this PR well enough to green-light anything, but I'll try and find some time to rebase over the next few days. |
Sorry, something went wrong.
Adds the documented but missing `retrying` event, and adds a `failed:fatal` event that signals that a job has failed permanently. The `failed` event remains as it was, signaling job execution failures without signaling the outcome of the job. Co-authored-by: Hugh Secker-Walker <hsw@hodain.net>
|
Rebased, largely to reconcile against the changes in #134. Unfortunately, it seems the tests are flaky. I don't know if this is a new upstream regression, or something that changed in this branch, but I don't have the bandwidth to diagnose that. It's especially difficult to debug in this project because ava doesn't report which test(s) were responsible for the timeout (or at least, that used to be its behavior, and I don't see any obvious change in the output that would suggest otherwise). |
Sorry, something went wrong.
Could you describe your setup? I'm getting intermittent and inconsistent test failures when I run the tests locally on my M1 MacBook Pro with Redis 6.2 and Node 16.17.1. Strangely, they pass consistently when run in CI. Not sure what's up with that, more data may suggest a solution. |
Sorry, something went wrong.
|
@jorenvandeweyer please add your review! Thanks :) |
Sorry, something went wrong.
Ah, this is in CI. It (seemed to) pass consistently in my local setup. Just a redis-server on an intel mac running macOS 11.7.1. |
Sorry, something went wrong.
@compwright I'm seeing the same inconsistent test failures on my M1 Pro (I'm using DBngin). I suggest you run the tests using Docker for now. |
Sorry, something went wrong.
## [1.7.0](v1.6.0...v1.7.0) (2023-11-06) ### Features * improve job failure events ([#227](#227)) ([15d02c2](15d02c2))
|
🎉 This PR is included in version 1.7.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sorry, something went wrong.
Adds the documented but missing retrying event, and adds a failed:fatal event that signals that
a job has failed permanently. The failed event remains as it was, signaling job execution
failures without signaling the outcome of the job.
Fixes #184; see #186 for more discussion and an alternate proposal.