Repository navigation
Consider mapped upstream in TriggerRuleDep - #22583
Merged
Merged
Conversation
ashb
reviewed
Mar 29, 2022
uranusjr
force-pushed
the
mapped-task-reduce-downstream
branch
5 times, most recently
from
March 30, 2022 17:49
28ec197 to
85ab997
Compare
uranusjr
commented
Mar 30, 2022
Comment on lines
822
to
846
Member
Author
There was a problem hiding this comment.
Somewhat surprising we didn’t catch this earlier!
uranusjr
marked this pull request as ready for review
March 30, 2022 17:50
dstandish
reviewed
Mar 31, 2022
uranusjr
force-pushed
the
mapped-task-reduce-downstream
branch
from
March 31, 2022 05:58
85ab997 to
318367f
Compare
ashb
reviewed
Mar 31, 2022
dstandish
reviewed
Mar 31, 2022
Contributor
There was a problem hiding this comment.
this reminds me... thinking about custom xcom backends ... there should be a probably be a "clear data" hook that can be implemented in an xcom backend to purge the external data without having to override this method... of course that's another PR
Member
Author
There was a problem hiding this comment.
Is XCom.clear() not enough?
uranusjr
force-pushed
the
mapped-task-reduce-downstream
branch
from
April 1, 2022 06:47
10d712a to
2b4d9a2
Compare
ashb
reviewed
Apr 1, 2022
uranusjr
force-pushed
the
mapped-task-reduce-downstream
branch
from
April 1, 2022 10:11
2b4d9a2 to
6a9f8bd
Compare
ashb
approved these changes
Apr 1, 2022
Contributor
|
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
github-actions
Bot
added
the
full tests needed
This is needed to ensure all mapped upstream task instances are depended on by a downstream, instead of just one of them signifying the entire task as "done".
We catually missed quite several things for this to happen! Most namely the pre-execution XCom clearing behavior; that's surprising. But things should work now.
uranusjr
force-pushed
the
mapped-task-reduce-downstream
branch
from
April 4, 2022 12:31
6a9f8bd to
5a62d56
Compare
ashb
approved these changes
Apr 4, 2022
ephraimbuddy
added
the
changelog:skip
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is needed to ensure all mapped upstream task instances are depended on by a downstream, instead of just one of them signifying the entire task as "done".
Does not work yet. This depends on correct task-unmapping behaviour implemented in #22396. I also want to add some more asserts to make sure the downstream can pull all the values from the mapped upstream, but that depends on #22609.Ready!