feat(job): provide decrementAttemptsMade option in moveToDelayed #2203
Conversation
|
It's desired to keep the attemptMade up today, we may need to think more about it |
roggervalf
changed the title
src/classes/job.ts
Outdated
| moveToDelayed( | ||
| timestamp: number, | ||
| token?: string, | ||
| skipAttempt?: boolean, |
There was a problem hiding this comment.
Instead of having booleans it is much more descriptive to use an options object with a skipAttempt option in it.
src/classes/scripts.ts
Outdated
| @@ -683,6 +685,7 @@ export class Scripts { | |||
| jobId, | |||
| token, | |||
| delay, | |||
| opts.decrementAttempt ? '1' : '0', | |||
There was a problem hiding this comment.
I find this a bit awkward, in the sense that we must decrement to effectively achieve a non-increment of attemptsMade. I think it would be better to not increment instead of increment and then decrement.
There was a problem hiding this comment.
The problem is probably that we increment attemptsMade as soon as the job is moved to active, but maybe we should increment this property when the job is completed or failed.
There was a problem hiding this comment.
When a job is moved to active we have not "made an attempt" we have "started an attempt", but it is not until the job finishes we should consider the attempt to be made, what do you think?
There was a problem hiding this comment.
we may not only consider when it completes or fails, also when it's moved to delayed, retried script and movedToWaitingChildren
There was a problem hiding this comment.
We should think that "attemptsMade" main use is for the error retries, and the backoff function. So actually increasing it when completing the job or failing would work well in this regard. On the other hand, it is also good to know how many times a job has been started, but may for some reason not finished, like it was considered stalled and moved back to wait, or it has been considered rate-limited, and so on.
roggervalf
changed the base branch from
master
to
feat/better-handling-of-queue-markers
roggervalf
changed the base branch from
feat/better-handling-of-queue-markers
to
master
roggervalf
changed the base branch from
master
to
feat/better-handling-of-queue-markers
| ); | ||
|
|
||
| if (!opts.skipAttempt) { |
There was a problem hiding this comment.
What is the scenario where skipAttempt is necessary?
# [5.0.0](v4.17.0...v5.0.0) (2023-12-21) ### Bug Fixes * **connection:** require connection to be passed ([#2335](#2335)) ([1867dd1](1867dd1)) * **job:** revert console warn custom job ids when they represent integers ([#2312](#2312)) ([84015ff](84015ff)) * **python:** unify redis connection args for Queue and Worker ([#2282](#2282)) ([8eee20f](8eee20f)) * **worker:** throw error if connection is missing ([6491a18](6491a18)) ### Features * **job:** provide skipAttempt option when manually moving a job ([#2203](#2203)) ([0e88e4f](0e88e4f)) * **python:** use new queue markers ([b0a13e8](b0a13e8)) * **python:** use new queue markers ([4276eb7](4276eb7)) * **worker:** improved markers handling ([73cf5fc](73cf5fc)) * **worker:** improved markers handling ([0bac0fb](0bac0fb)) ### BREAKING CHANGES * **worker:** Markers use now a dedicated key in redis instead of using a special Job ID. * **worker:** Markers use now a dedicated key in redis instead of using a special Job ID.
ref #2152