Many fixes for handling tasks in jobs and executors #1271
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jlowin
changed the title
|
@jlowin this is a really great stuff! What I would really like (if you need help let me know), is to have some tests in place especially for the ignore_depends_on_past parameter as we have been bitten by this before, the schedulerjob fix and the deadlock ones. This would increase coverage in this area which is pretty important. |
jlowin
force-pushed
the
subdag_pool_issue_1225
branch
2 times, most recently
from
11d75d6 to
534ca3a
Compare
Use the same calling format as the other Executors
The current machinery for running BackfillJobs overrides tasks’ start_dates to deal with depends_on_past. This is fragile and, critically, doesn’t always carry through all of the nested Jobs. We replace it with an explicit instruction to ignore_depends_on_past when considering whether a task can be queued. Also, this will be used later to evaluate whether a set of tasks is deadlocked.
1. Introduce a concept of a deadlocked backfill, meaning no tasks can run. The easiest way to create this is with depends_on_past. Previously, backfill would sit forever. Now it identifies the deadlock and exist, possibly informing the user that a cause of the deadlock with depends_on_past 2. Previously, BackfillJob would run a task once to put it in a queue, but then ignore the queued task on every subsequent loop, resulting in it never being run. Now it considers queued tasks and runs them. 3. “UP_FOR_RETRY” tasks were not handled properly by the executor (it raised the “the airflow run command failed at reporting an error” message). Now they are.
SchedulerJob loads EVERY queued task and tries to run it, which creates conflicts with any other Job trying to do the same (BackfillJob from CLI or subdag, or potentially [one day] other schedulers). This creates a new method, process_events, which polls the Scheduler’s own executor for queued tasks and adds them to a set. The scheduler then only considers that set when prioritizing queued tasks.
Previously, DagRuns failed if any task failed and succeeded if all tasks succeeded or were skipped. However, because of trigger behaviors, that’s not right — a task can fail and another task can start up with an “on failed” trigger. This changes the logic to consider three termination cases: 1. Failure. If any of the root tasks fail, the dagrun fails. This is because there is no possibility of any “on failure” trigger coming off a root task. 2. Success. If ALL of the root nodes succeed or skip, the dagrun succeeds. This means there can be upstream failures as long as failure triggers are respected. 3. Deadlock — A dag run is deadlocked when no action is possible. This is determined by the presence of unfinished tasks without met dependencies. However, care must be taken when depends_on_past=True because individual dag runs could *look* like they are deadlocked when they are actually just waiting for earlier runs to finish. To solve this problem, we evaluate deadlocks in two ways. First, across all dagruns simultaneously (to account for situations with depends_on_past=True). Second, in each individual dagrun (but only if there are no depends_on_past relationship).
jlowin
force-pushed
the
subdag_pool_issue_1225
branch
from
534ca3a to
68b236d
Compare
|
|
Fix minor issues including:
- clean up State
- fix bug with nonstandard DAGS_FOLDER locations
- remove restriction on dags not being outside DAGS_FOLDER
because DagBags are allowed to load dags from anywhere
- miscellaneous Landscape fixes
- use logger instead of print for DAG.clear()
jlowin
force-pushed
the
subdag_pool_issue_1225
branch
from
68b236d to
c1eb83a
Compare
|
|
This was referenced Apr 3, 2016
|
|
This was referenced Apr 4, 2016
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The grand sum of fixes from #1225, also closes #1254 and #1255.
This is a big PR and I've left the commits separated for clarity. When merged, I can squash them (though it may make sense to leave them distinct because they address many different issues).
The comments for each commit will give the details, but at a high level this address a few things:
BackfillJobSchedulerJobevents_buffer.BackfillJobdepends_on_pastinBackfillJobdepends_on_pastdependencies when determining if a task can run. Users can tell a BackfillJob that they want toignore_first_depends_on_pastif they desire. The default is not to do this (but the helpful deadlock error message will tell users if that's the reason their tasks aren't running).depends_on_pastis-Ior--ignore_depends_on_past(forairflow run) or--ignore_first_depends_on_past(forairflow backfill)