-
Notifications
You must be signed in to change notification settings - Fork 13.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extend documentation for states of DAGs & tasks and update trigger rules docs #21382
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
potiuk
reviewed
Feb 7, 2022
potiuk
reviewed
Feb 7, 2022
mnojek
requested review from
ashb,
mik-laj,
jedcunningham,
turbaszek,
uranusjr,
jhtimmins,
ryanahamilton,
bbovenzi,
ephraimbuddy,
dstandish and
XD-DENG
as code owners
February 15, 2022 16:23
mnojek
force-pushed
the
update-trigger-rules-docs
branch
from
February 15, 2022 16:25
d0a0291
to
1b47208
Compare
potiuk
approved these changes
Feb 21, 2022
github-actions
bot
added
the
okay to merge
It's ok to merge this PR as it does not require more tests
label
Feb 21, 2022
The PR is likely ready to be merged. No tests are needed as no important environment files, nor python files were modified by it. However, committers might decide that full test matrix is needed and add the 'full tests needed' label. Then you should rebase it to the latest main or amend the last commit of the PR, and push it with --force-with-lease. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
kind:documentation
okay to merge
It's ok to merge this PR as it does not require more tests
type:doc-only
Changelog: Doc Only
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The story behind this change:
This change is somehow inspired by the AIP-47 that is still in the discussion phase. In this new design of system tests, we need to make use of Trigger Rules for handling
teardown
tasks (e.g. for cleaning resources required by recently executed the system test). The main concern is that when we useteardown
task with trigger ruleall_done
(to make sure that it is executed even if something's gone wrong in the middle of the test), the whole test (which is just a DAG) will take the result from this particularteardown
task and we can lose the information about some failing task in the middle. This is not expected workflow for the tests, because we want the test to fail if any step (task) failed. The reason why the whole test gets the status of ateardown
task and not signalizes that anything failed in the middle is that Airflow works like this - the DAG Run status is determined by the status of the "leaf nodes" (the tasks that do not have any children). Since theteardown
task is the leaf node, the whole test gets the same status (which is almost alwayssuccess
).That's why we need to have another
watcher
task with trigger rule set toone_failed
that is a downstream task for any other task in the test (DAG). Thanks to this, it will be triggered if any of the task in the DAG failed and thus its status will be propagated to the DAG Run (because it is a leaf node).By doing the research in the documentation and code, I found it very difficult to find the information how trigger rules work in the details and that's why I thought that it would be good to extend the documentation for them. Since I already spent some time to understand it deeply, I also took the effort and prepared this PR. It's not big, but it took me several hours to prepare these statements. I am not sure if all the statements are correct, so please read it carefully and correct me if I'm wrong and I will edit the PR.
On the other hand, I would like to also start a discussion about the trigger rules. To me they seemed simple at first, but the more time I spent with them I figured out that they introduce a lot of complications to the task execution. I hope that this PR will make it easier to understand how they work. If you have any ideas how we can make them even better, I am glad to discuss it.