[AIRFLOW-4453] Make behavior of none_failed consistent with documentation (#7464)
#235
Conversation
…tation (apache#7464) The documentation for the `none_failed` trigger rule (https://airflow.apache.org/docs/stable/concepts.html#trigger-rules) describes its behavior as "all parents have not failed (`failed` or `upstream_failed`) i.e. all parents have succeeded or been skipped." With that definition in mind, there is no reason that the check for `none_failed` should ever have to check for skipped upstream tasks or set the current task to `skipped`. The current behavior causes the rule to skip if all upstream tasks have skipped, which is more than a little confusing. This fixes the behavior to be consistent with the documentation. Co-authored-by: root <[email protected]>
Codecov Report
@@ Coverage Diff @@
## airflowinfra #235 +/- ##
================================================
- Coverage 27.82% 27.81% -0.01%
================================================
Files 528 529 +1
Lines 36654 36679 +25
================================================
+ Hits 10198 10202 +4
- Misses 26456 26477 +21
Continue to review full report at Codecov.
|
| @@ -211,6 +214,15 @@ def _evaluate_trigger_rule( | |||
| "upstream_tasks_state={2}, upstream_task_ids={3}" | |||
| .format(tr, num_failures, upstream_tasks_state, | |||
| task.upstream_task_ids)) | |||
| elif tr == TR.NONE_FAILED_OR_SKIPPED: | |||
| num_failures = upstream - successes - skipped | |||
There was a problem hiding this comment.
So actually the name should be all_success_or_skipped to be more accurate?
There was a problem hiding this comment.
I'm not entirely sure. I'm kind of confused how it differs from NONE_FAILED, but no one seems to have an issue with it upstream :/
There was a problem hiding this comment.
Yeah the implementation looks exactly the same :/
There was a problem hiding this comment.
Actually no. For NONE_FAILED_OR_SKIPPED, the task won't be skipped if there is at least one success
|
💨 |
The documentation for the
none_failedtrigger rule(https://airflow.apache.org/docs/stable/concepts.html#trigger-rules)
describes its behavior as "all parents have not failed (
failedorupstream_failed) i.e. all parents have succeeded or been skipped."With that definition in mind, there is no reason that the check for
none_failedshould ever have to check for skipped upstream tasks orset the current task to
skipped. The current behavior causes the ruleto skip if all upstream tasks have skipped, which is more than a little
confusing. This fixes the behavior to be consistent with the documentation.
Co-authored-by: root [email protected]
Issue link: WILL BE INSERTED BY boring-cyborg
Make sure to mark the boxes below before creating PR: [x]
[AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID** For document-only changes commit message can start with
[AIRFLOW-XXXX].In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.
Based on this PR:
https://github.com/lyft/etl/pull/32271