You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "dstandish (via GitHub)" <gi...@apache.org> on 2023/06/02 05:55:45 UTC
[GitHub] [airflow] dstandish opened a new pull request, #31676: Correct dagrun state logic when dag is all teardowns
dstandish opened a new pull request, #31676:
URL: https://github.com/apache/airflow/pull/31676
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #31676: Correct dagrun state logic when dag is all teardowns
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #31676:
URL: https://github.com/apache/airflow/pull/31676#discussion_r1214051151
##########
airflow/models/dagrun.py:
##########
@@ -552,6 +552,9 @@ def is_effective_leaf(task):
return not task.is_teardown or task.on_failure_fail_dagrun
leaf_task_ids = {x.task_id for x in dag.tasks if is_effective_leaf(x)}
+ 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}
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #31676: Correct dagrun state logic when dag is all teardowns
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #31676:
URL: https://github.com/apache/airflow/pull/31676#discussion_r1214010523
##########
airflow/models/dagrun.py:
##########
@@ -552,6 +552,9 @@ def is_effective_leaf(task):
return not task.is_teardown or task.on_failure_fail_dagrun
leaf_task_ids = {x.task_id for x in dag.tasks if is_effective_leaf(x)}
+ 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}
Review Comment:
Yes you need something to infer dag run state
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish merged pull request #31676: Correct dagrun state logic when dag is all teardowns
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish merged PR #31676:
URL: https://github.com/apache/airflow/pull/31676
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on a diff in pull request #31676: Correct dagrun state logic when dag is all teardowns
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #31676:
URL: https://github.com/apache/airflow/pull/31676#discussion_r1214008734
##########
airflow/models/dagrun.py:
##########
@@ -552,6 +552,9 @@ def is_effective_leaf(task):
return not task.is_teardown or task.on_failure_fail_dagrun
leaf_task_ids = {x.task_id for x in dag.tasks if is_effective_leaf(x)}
+ 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}
Review Comment:
Why do we need a non-empty list of leaves? Does it affect the DAG run state or serve a different purpose?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #31676: Correct dagrun state logic when dag is all teardowns
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #31676:
URL: https://github.com/apache/airflow/pull/31676#discussion_r1214051151
##########
airflow/models/dagrun.py:
##########
@@ -552,6 +552,9 @@ def is_effective_leaf(task):
return not task.is_teardown or task.on_failure_fail_dagrun
leaf_task_ids = {x.task_id for x in dag.tasks if is_effective_leaf(x)}
+ 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}
Review Comment:
Yeah it's an odd case. Very much nonsensical edge case that doesn't really make sense. If there's no no 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on a diff in pull request #31676: Correct dagrun state logic when dag is all teardowns
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #31676:
URL: https://github.com/apache/airflow/pull/31676#discussion_r1214021242
##########
airflow/models/dagrun.py:
##########
@@ -552,6 +552,9 @@ def is_effective_leaf(task):
return not task.is_teardown or task.on_failure_fail_dagrun
leaf_task_ids = {x.task_id for x in dag.tasks if is_effective_leaf(x)}
+ 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}
Review Comment:
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 :thinking:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org