Fix teardown scheduling if indirect upstream is cleared #38788
Conversation
jscheffl
added
area:Scheduler
jscheffl
changed the title
jscheffl
force-pushed
the
bugfix/29332-fix-teardown-with-direct-upsteam-success
branch
from
bc7dfae
to
31f21f2
Compare
| 2, | ||
| _UpstreamTIStates(6, 0, 1, 0, 0, 7, 1, 0), | ||
| [SUCCESS, SUCCESS, SUCCESS, SUCCESS, SUCCESS, SUCCESS, FAILED], |
There was a problem hiding this comment.
This pytest fails because it seems the dependency context is returned empty after evaluation. I am not sure why. I assume something is not "correctly mocked".
| 2, | ||
| _UpstreamTIStates(6, 1, 0, 0, 0, 7, 1, 1), | ||
| [SUCCESS, SUCCESS, SUCCESS, SUCCESS, SUCCESS, SUCCESS, SKIPPED], |
There was a problem hiding this comment.
This pytest fails because it seems the dependency context is returned empty after evaluation. I am not sure why. I assume something is not "correctly mocked".
| True, | ||
| (False, "skipped"), # is_teardown=False, expect_state="skipped' | ||
| True, | ||
| id="not teardown all done one skipped", |
There was a problem hiding this comment.
In this test I would have expected a skipped result but it is returning with Noneafter evaluation. I assume some mocks are wrong here :-(
| for i in range(upstream_setups): | ||
| task = EmptyOperator(task_id=f"setup_{i}", dag=dag).as_setup() | ||
| task.set_downstream(downstream) | ||
| task.set_downstream(work_tasks) |
There was a problem hiding this comment.
Note: I re-modelled the test DAG such that the setup tasks are in front of the worker tasks, else the setup is directly linked to downstream teardown which (1) does not make sense and (2) is explicitly not handled.
There was a problem hiding this comment.
i don't understand. it might be best to make a separate PR for your test refactor without simultaneously changing the code. then followup with the real code change (and the test changes required just for that will be easier to see)
| ["one_success", 0, _UpstreamTIStates(0, 0, 5, 0, 0, 5, 0, 0), True, State.UPSTREAM_FAILED, False], | ||
| ["one_success", 0, _UpstreamTIStates(0, 0, 4, 1, 0, 5, 0, 0), True, State.UPSTREAM_FAILED, False], | ||
| ["one_success", 0, _UpstreamTIStates(0, 0, 0, 5, 0, 5, 0, 0), True, State.UPSTREAM_FAILED, False], | ||
| ["one_success", 0, [SUCCESS, SUCCESS, SUCCESS, SUCCESS, SUCCESS], True, None, True], |
There was a problem hiding this comment.
i'm not a super huge fan of the scrutability of the "old" way, but it's hard to see how the new way tests the same thing. like i said below, i think best to separate test change from code+test change
This PR fixes the calculation of the scheduling decision if upstream to a teardown task an individual task is reset (w/o resetting all downstream!) but the direct upstream tasks of the teardown are completed.
As reported in #29332 in such case as direct upstream dependencies of teardown tasks were completed, teardown was starting too early.
With this fix teardown scheduling evaluates all upstream dependencies if any upstream is not completed.
How to test?
closes: #29332