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

Prevents batch tasks from running before their dependencies #1846

Merged
merged 1 commit into from
Sep 12, 2016

Conversation

daveFNbuck
Copy link
Contributor

Description

Checks whether batch tasks are schedulable before adding them to a batch in get_work.

Note that even though self._schedulable(task) gets called twice in the get_work loop, they fall under different branches of whether best_task is defined, so this shouldn't slow get_work down much.

Motivation and Context

When adding tasks to an existing batch in get_work, we neglected to check whether the tasks were schedulable. This meant that in production I found my pipelines running batches that included jobs with PENDING dependencies. In order to fix this, we simply add a check that the task is schedulable.

Have you tested this? If so, how?

I have included unit tests.

When adding tasks to an existing batch in get_work, we neglected to check
whether the tasks were schedulable. This meant that in production I found
my pipelines running batches that included jobs with PENDING dependencies.
In order to fix this, we simply add a check that the task is schedulable.

Note that even though self._schedulable(task) gets called twice in the
get_work loop, they fall under different branches of whether best_task is
defined, so this shouldn't slow get_work down much.
@Tarrasch Tarrasch merged commit 70d8734 into spotify:master Sep 12, 2016
@Tarrasch
Copy link
Contributor

Thanks. Am I correct in that this is a pretty serious bug as it would run a task before it's dependencies are complete?

@daveFNbuck daveFNbuck deleted the prevent_stowaways branch September 12, 2016 17:07
@daveFNbuck
Copy link
Contributor Author

Yes, that was what happened in production after I deployed a version of Luigi with this bug.

This was referenced Jun 29, 2022
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.

2 participants