-
Notifications
You must be signed in to change notification settings - Fork 419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(job): add removeDependencyOnFail option #1100
Changes from 9 commits
b53f9b6
0658553
a81ff7e
28d399a
8b6655e
b99b381
721a0d4
2c3c485
dcbebd0
516036b
48206ba
978e732
243c4dc
0db60cf
a3cadcc
f0a0765
28243a5
ae5a86e
42bfea7
cf664aa
151805a
eba4ced
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,12 +5,16 @@ import { | |
BackoffOptions, | ||
JobJson, | ||
JobJsonRaw, | ||
JobsOptions, | ||
ParentKeys, | ||
RedisClient, | ||
WorkerOptions, | ||
} from '../interfaces'; | ||
import { JobState, JobJsonSandbox } from '../types'; | ||
import { | ||
JobsOptions, | ||
JobState, | ||
JobJsonSandbox, | ||
RedisJobOptions, | ||
} from '../types'; | ||
import { | ||
errorObject, | ||
isEmpty, | ||
|
@@ -35,6 +39,7 @@ export interface MoveToChildrenOpts { | |
}; | ||
} | ||
|
||
type ValueOf<T> = T[keyof T]; | ||
export interface DependenciesOpts { | ||
processed?: { | ||
cursor?: number; | ||
|
@@ -233,7 +238,7 @@ export class Job< | |
jobId?: string, | ||
): Job<T, R, N> { | ||
const data = JSON.parse(json.data || '{}'); | ||
const opts = JSON.parse(json.opts || '{}'); | ||
const opts = Job.optsFromJSON(json.opts); | ||
|
||
const job = new this<T, R, N>( | ||
queue, | ||
|
@@ -276,6 +281,28 @@ export class Job< | |
return job; | ||
} | ||
|
||
static optsFromJSON(rawOpts?: string): JobsOptions { | ||
const opts = JSON.parse(rawOpts || '{}'); | ||
|
||
const optionEntries = Object.entries(opts) as Array< | ||
[keyof RedisJobOptions, any] | ||
>; | ||
const options = optionEntries.reduce<Partial<Record<string, any>>>( | ||
(acc, item) => { | ||
const [attributeName, value] = item; | ||
if (attributeName === 'rdof') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Works for now but I guess in the future we will have a map object for translating all the options from Redis representation to library representation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, this is the beginning of it |
||
acc.removeDependencyOnFail = value; | ||
} else { | ||
acc[attributeName] = value; | ||
} | ||
return acc; | ||
}, | ||
{}, | ||
); | ||
|
||
return options as JobsOptions; | ||
} | ||
|
||
/** | ||
* Fetches a Job from the queue given the passed job id. | ||
* | ||
|
@@ -311,7 +338,7 @@ export class Job< | |
id: this.id, | ||
name: this.name, | ||
data: JSON.stringify(typeof this.data === 'undefined' ? {} : this.data), | ||
opts: this.opts, | ||
opts: this.optsAsJSON(this.opts), | ||
progress: this.progress, | ||
attemptsMade: this.attemptsMade, | ||
finishedOn: this.finishedOn, | ||
|
@@ -323,6 +350,26 @@ export class Job< | |
}; | ||
} | ||
|
||
private optsAsJSON(opts: JobsOptions = {}): RedisJobOptions { | ||
const optionEntries = Object.entries(opts) as Array< | ||
[keyof JobsOptions, any] | ||
>; | ||
const options = optionEntries.reduce<Partial<Record<string, any>>>( | ||
(acc, item) => { | ||
const [attributeName, value] = item; | ||
if (attributeName === 'removeDependencyOnFail') { | ||
acc.rdof = value; | ||
} else { | ||
acc[attributeName] = value; | ||
} | ||
return acc; | ||
}, | ||
{}, | ||
); | ||
|
||
return options as RedisJobOptions; | ||
} | ||
|
||
/** | ||
* Prepares a job to be passed to Sandbox. | ||
* @returns | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,13 @@ | ||
--[[ | ||
Validate and move or add dependencies to parent. | ||
Add processed results, validate and move parent to active if needed. | ||
]] | ||
|
||
-- Includes | ||
--- @include "updateParentIfNeeded" | ||
|
||
local function updateParentDepsIfNeeded(parentKey, parentQueueKey, parentDependenciesKey, | ||
parentId, jobIdKey, returnvalue ) | ||
local processedSet = parentKey .. ":processed" | ||
rcall("HSET", processedSet, jobIdKey, returnvalue) | ||
local activeParent = rcall("ZSCORE", parentQueueKey .. ":waiting-children", parentId) | ||
if rcall("SCARD", parentDependenciesKey) == 0 and activeParent then | ||
rcall("ZREM", parentQueueKey .. ":waiting-children", parentId) | ||
if rcall("HEXISTS", parentQueueKey .. ":meta", "paused") ~= 1 then | ||
rcall("RPUSH", parentQueueKey .. ":wait", parentId) | ||
else | ||
rcall("RPUSH", parentQueueKey .. ":paused", parentId) | ||
end | ||
local parentEventStream = parentQueueKey .. ":events" | ||
rcall("XADD", parentEventStream, "*", "event", "active", "jobId", parentId, "prev", "waiting-children") | ||
end | ||
updateParentIfNeeded(parentQueueKey, parentDependenciesKey, parentId ) | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
--[[ | ||
Validate and move parent to active if needed. | ||
]] | ||
|
||
local function updateParentIfNeeded(parentQueueKey, parentDependenciesKey, parentId ) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could this function be renamed to "moveParentToWaitifNeeded" ? seems like that is what it does, "update" it is a big vague and makes it more difficult to understand. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good to me |
||
local activeParent = rcall("ZSCORE", parentQueueKey .. ":waiting-children", parentId) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, shouldn't "activeParent" variable name be called instead something like: "isParentWaiting" ? |
||
if rcall("SCARD", parentDependenciesKey) == 0 and activeParent then | ||
rcall("ZREM", parentQueueKey .. ":waiting-children", parentId) | ||
if rcall("HEXISTS", parentQueueKey .. ":meta", "paused") ~= 1 then | ||
rcall("RPUSH", parentQueueKey .. ":wait", parentId) | ||
else | ||
rcall("RPUSH", parentQueueKey .. ":paused", parentId) | ||
end | ||
local parentEventStream = parentQueueKey .. ":events" | ||
rcall("XADD", parentEventStream, "*", "event", "active", "jobId", parentId, "prev", "waiting-children") | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
export * from './finished-status'; | ||
export * from './job-json-sandbox'; | ||
export * from './job-options'; | ||
export * from './job-type'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import { JobOptionsBase } from '../interfaces'; | ||
|
||
export type JobsOptions = JobOptionsBase & { | ||
/** | ||
* If true, removes the job from its parent dependencies when it fails after all attempts. | ||
*/ | ||
removeDependencyOnFail?: boolean; | ||
}; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe a comment here clarifying that these fields are the ones stored in Redis and thus with smaller keys for compactness. |
||
export type RedisJobOptions = JobOptionsBase & { | ||
/** | ||
* If true, removes the job from its parent dependencies when it fails after all attempts. | ||
*/ | ||
rdof?: boolean; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this method should be private since it has no use outside of this class.