Skip to content
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

Closed
wants to merge 22 commits into from

Conversation

roggervalf
Copy link
Collaborator

@roggervalf roggervalf commented Feb 24, 2022

ref #800

@roggervalf roggervalf requested a review from manast March 1, 2022 02:55
Copy link
Contributor

@manast manast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, some small comments.

*/
removeDependencyOnFail?: boolean;
};

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -276,6 +281,28 @@ export class Job<
return job;
}

static optsFromJSON(rawOpts?: string): JobsOptions {
Copy link
Contributor

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.

const options = optionEntries.reduce<Partial<Record<string, any>>>(
(acc, item) => {
const [attributeName, value] = item;
if (attributeName === 'rdof') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this is the beginning of it

]]

local function updateParentIfNeeded(parentQueueKey, parentDependenciesKey, parentId )
local activeParent = rcall("ZSCORE", parentQueueKey .. ":waiting-children", parentId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, shouldn't "activeParent" variable name be called instead something like: "isParentWaiting" ?

Validate and move parent to active if needed.
]]

local function updateParentIfNeeded(parentQueueKey, parentDependenciesKey, parentId )
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good to me

@roggervalf roggervalf requested a review from manast March 3, 2022 00:41
@M3talen
Copy link

M3talen commented Dec 14, 2022

Dear @manast ,

I wanted to reach out and express my appreciation for the work you have done on the this project. I have been using the project for a few projects, and I think it is a fantastic resource.

I was wondering if you would be open to considering the addition of this feature that I believe would greatly benefit the project. As right now there is no other way to complete a flow without removing the child manually. But the bigger problem for me is that removing child's from the array doesn't update the flow job.

Thank you for your time, and I look forward to hearing from you.

Sincerely,
@M3talen

@roggervalf
Copy link
Collaborator Author

hi @M3talen,
could you please elaborate a little bit your use case, now I'm thinking that if a job is not required to be completed for an specific flow, this should mean that the parent-child relationship is not needed in that case. Also as you mentioned, when a child is deleted, the parent should be moved to wait if this is the last pending child for that job as in https://github.com/taskforcesh/bullmq/blob/master/tests/test_flow.ts#L2480-L2525.

@M3talen
Copy link

M3talen commented Dec 16, 2022

hi @roggervalf
It might be a bit difficult to explain but ill try to show it off

  1. in EngineQueue i create a new task (daily-flow)
  2. that task creates a new flow with a parent called 'flow-job' in a new queue (FlowQueue)
  3. as children i put tasks that need to be done daily in other queues
    3.1) Flow starts executing first children.
    3.2) job in EngineQueue gets put to completed. (here start the problems - if i wait for this job and a child fails it will stop procesing the queue)
    Now i don't care about what tasks fail but when one fails i would like to add a stacktrace/log/update data on the 'flow-job' so i know what failed but i want the 'flow-job' to be executed when all children either fail or complete.

Currently when a task fails (a child) the 'flow-job' is not executed until i delete it
even if the failed task fails all attempts and gets moved to failed it doesn't execute the 'flow-job'

I hope this explains a bit of my problem.

@roggervalf
Copy link
Collaborator Author

hi @M3talen, in your case you can use this function https://api.docs.bullmq.io/classes/FlowProducer.html#addBulk, for example:

const trees = await flow.addBulk([
        {
          name: 'flow-job',
          queueName: 'Flow-Queue',
          data: {},
          children: [
            {
              name,
              data: { idx: 0, foo: 'bar' },
              queueName: 'Flow-Queue',
            },
          ],
        },
        {
          name: 'non-dependent-child',
          queueName: 'Flow-Queue',
          data: {},
          children: [
            {
              name,
              data: { idx: 1, foo: 'baz' },
              queueName: 'Flow-Queue',
            },
          ],
        },
      ]);

so when you have children where you don't care if they complete or fail, those could be created apart of the main flow, with addBulk you can create all those flows together

@roggervalf
Copy link
Collaborator Author

in favor of #1953

@roggervalf roggervalf closed this Jun 7, 2023
@roggervalf roggervalf deleted the remove-dependency-on-fail branch June 7, 2023 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants