-
Notifications
You must be signed in to change notification settings - Fork 427
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
perf(scheduler): create tasks in parallel #2373
Conversation
} else { | ||
taskIds.push(task.value.id); | ||
taskIds.push(taskRes.value.value.id); |
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.
a bit weird but the PromiseAllSettled result type is PromiseSettleResult<Result<Task>>[]
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.
Indeed. Looks good, just an open comment about the transaction
for (const taskRes of res) { | ||
if (taskRes.status === 'rejected') { | ||
logger.error(`Failed to schedule task: ${taskRes.reason}`); | ||
} else if (taskRes.value.isErr()) { | ||
logger.error(`Failed to schedule task: ${stringifyError(taskRes.value.error)}`); |
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.
you could have done all of that inside the .map (if that makes more sense)
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.
you mean with a then
? The goal is not to await for the result in the map
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.
I meant keeping the code exactly the same but inside the map, since you don't kill the transaction
schedules.value.map(async (schedule) => {
const task = await tasks.create(trx, {
scheduleId: schedule.id,
startsAfter: new Date(),
name: `${schedule.name}:${new Date().toISOString()}`,
createdToStartedTimeoutSecs: schedule.createdToStartedTimeoutSecs,
startedToCompletedTimeoutSecs: schedule.startedToCompletedTimeoutSecs,
heartbeatTimeoutSecs: schedule.heartbeatTimeoutSecs
});
if (task.isErr())
[...]
});
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.
I see. But you are awaiting while iterating through the list. no?
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.
should not no, this is a common pattern await Promise.all(array.map(async))
logger.error(`Failed to create task for schedule: ${schedule.id}`); | ||
}) | ||
); | ||
const res = await Promise.allSettled(createTasks); |
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.
Wait my comment was not sent 🤔
I was saying (before and after this pr) you are not killing the transaction if any tasks fails, is it on purpose?
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.
yes it is on purpose. Failing to create a task should not prevent other tasks to be created.
The same scheduling logic will try to create the task again in the next iteration if needed
4d2640f
to
b69417b
Compare
Describe your changes
Tasks are currently created by scheduler one by one. We can parallelize the tasks creation to speed up the scheduling transaction
Checklist before requesting a review (skip if just adding/editing APIs & templates)