ChrisEverling commented on a change in pull request #4182: [AIRFLOW-3336] Add 
new TriggerRule that will consider skipped ancestors as success 
URL: https://github.com/apache/airflow/pull/4182#discussion_r362862918
 
 

 ##########
 File path: airflow/ti_deps/deps/trigger_rule_dep.py
 ##########
 @@ -152,6 +152,11 @@ def _evaluate_trigger_rule(
             elif tr == TR.ONE_FAILED:
                 if upstream_done and not (failed or upstream_failed):
                     ti.set_state(State.SKIPPED, session)
+            elif tr == TR.NONE_FAILED:
+                if upstream_failed or failed:
+                    ti.set_state(State.UPSTREAM_FAILED, session)
+                elif skipped == upstream:
 
 Review comment:
   > Pls note that this can be a feature worth preserving, because if someone 
wants his task op2 to only run if at least some upstream tasks have succeeded 
(while others can be skipped), he can use "none_failed" to do that because 
that's exactly what this line does.
   > 
   > However, if he wants to have his task op2 succeed as long as none of the 
upstream tasks fail (either skipped for success), he can still use 
"none_failed" for it. The only workaround he needs to add is to introduce a 
DummyOperator that always succeeds directly upstream of op2.
   > 
   > So we can keep this feature, but we should make it clear in the doc that 
if all the upstream tasks are skipped, a task marked with "none_failed" will be 
skipped. Pls let me know. i don't mind creating a PR to update the doc.
   
   In my opinion, this isn't a reasonable workaround. I'm developing complex 
DAGs that consist of several tasks with only one upstream dependency-- all of 
which could be potentially skipped. The `none_failed` trigger rule seemed to be 
the perfect rule for my use case, but anytime I skip a task, skips propagate 
down until the next task with multiple upstream deps. Wouldn't the reason you 
mentioned to preserve this feature be satisfied by the ONE_SUCCESS trigger 
rule? Either way, I believe that `none_failed` communicates "trigger if all 
upstream are success or skipped" as your docs suggest.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
[email protected]


With regards,
Apache Git Services

Reply via email to