Skip to content

Correct dagrun state logic when dag is all teardowns#31676

Merged
dstandish merged 2 commits intoapache:mainfrom
astronomer:fix-case-when-dag-is-all-teardowns
Jun 2, 2023
Merged

Correct dagrun state logic when dag is all teardowns#31676
dstandish merged 2 commits intoapache:mainfrom
astronomer:fix-case-when-dag-is-all-teardowns

Conversation

@dstandish
Copy link
Contributor

@dstandish dstandish commented Jun 2, 2023

Small correction here, to handle the case when dag is all teardowns. In this case, we must simply consider the leaf tasks, whatever they are.

Also fixes issue in main from interaction of two recent prs

@dstandish dstandish requested review from XD-DENG, ashb and kaxil as code owners June 2, 2023 05:55
@dstandish dstandish requested a review from ephraimbuddy June 2, 2023 05:55
@dstandish dstandish force-pushed the fix-case-when-dag-is-all-teardowns branch from 50443e8 to d125252 Compare June 2, 2023 05:56
Comment on lines +555 to +557
if not leaf_task_ids:
# can happen if dag is exclusively teardown tasks
leaf_task_ids = {x.task_id for x in dag.tasks if not x.downstream_list}
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need a non-empty list of leaves? Does it affect the DAG run state or serve a different purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you need something to infer dag run state

Copy link
Member

Choose a reason for hiding this comment

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

So when the teardown task fails, we will fail the dag run even if on_failure_fail_dagrun is not True? I wonder if we can just consider the empty leaves list as all success without looking for the teardown tasks state 🤔

Copy link
Contributor Author

@dstandish dstandish Jun 2, 2023

Choose a reason for hiding this comment

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

Yeah it's an odd case. Very much nonsensical edge case that doesn't really make sense. If there's no non teardown tasks then there's no point in ignoring the teardowns. There's no point in even having tasks marked as teardowns in that case. So this seemed like a sensible fallback but we can discuss

@dstandish dstandish merged commit d59f2fa into apache:main Jun 2, 2023
@dstandish dstandish deleted the fix-case-when-dag-is-all-teardowns branch June 2, 2023 07:21
@dstandish dstandish added the AIP-52 Automatic setup and teardown tasks label Jun 23, 2023
@ephraimbuddy ephraimbuddy added this to the Airflow 2.7.0 milestone Jul 6, 2023
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AIP-52 Automatic setup and teardown tasks changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants