Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support clearing and updating state of individual mapped task instances #22958

Merged
merged 13 commits into from Apr 20, 2022
Merged

Support clearing and updating state of individual mapped task instances #22958

merged 13 commits into from Apr 20, 2022

Conversation

ephraimbuddy
Copy link
Contributor

@ephraimbuddy ephraimbuddy commented Apr 12, 2022 •
edited

Provide support for UI to clear/update state of mapped task instances

TODO:

  • Add test at UI side

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:webserver Webserver related Issues labels Apr 12, 2022
@bbovenzi bbovenzi changed the title Support clearing and updating state of mapped task instances Support clearing and updating state of individual mapped task instances Apr 13, 2022
@bbovenzi
Copy link
Contributor

Testing with the UI branch mapped-instance-actions. And everything appears to me working. 👍

@ephraimbuddy ephraimbuddy force-pushed the clearing-and-marking-mapped-task branch from 95a0a2e to 916ae06 Compare April 13, 2022 20:08
jedcunningham
jedcunningham reviewed Apr 13, 2022
View reviewed changes
airflow/api/common/mark_tasks.py Outdated Show resolved Hide resolved
airflow/models/dag.py Outdated Show resolved Hide resolved
uranusjr
uranusjr reviewed Apr 14, 2022
View reviewed changes
airflow/api/common/mark_tasks.py Outdated Show resolved Hide resolved
airflow/models/dag.py Outdated Show resolved Hide resolved
airflow/models/dag.py Outdated Show resolved Hide resolved
airflow/www/views.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Apr 15, 2022
@github-actions
Copy link

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.

@ephraimbuddy ephraimbuddy force-pushed the clearing-and-marking-mapped-task branch from 79ee7de to 98abe5f Compare April 15, 2022 11:35
@ephraimbuddy
Copy link
Contributor Author

Not sure how to fix some mypy errors here. @uranusjr can you help. Thanks

@uranusjr

This comment was marked as outdated.

Sign in to view
@uranusjr
Copy link
Member

uranusjr commented Apr 18, 2022 •
edited

I pushed a commit for this. The entire set_task_instance_state, get_task_isntances and clear combination is a mess, so I hope this keeps everything right-ish. We’ll see…

Edit: Most things seem to work, except MSSQL does not seem to like tuple_ :(

@uranusjr uranusjr force-pushed the clearing-and-marking-mapped-task branch 2 times, most recently from 02f3f6b to 45cb2d6 Compare April 18, 2022 15:30
@bbovenzi
Copy link
Contributor

bbovenzi commented Apr 18, 2022 •
edited

The latest changes got rid of map_indexes but we should still have an ability to pass multiple map indexes in a single request. Either comma separated like map_index=0%2C1 in the url or as multiple params like &map_index=0&map_index=1

@uranusjr
Copy link
Member

I added a commit to support &map_index=0&map_index=1 This is translated to [(task_id, 0), (task_id, 1)] before being passed to set_state.

@uranusjr uranusjr force-pushed the clearing-and-marking-mapped-task branch 2 times, most recently from 22581b4 to 0a52609 Compare April 19, 2022 10:04
bbovenzi
bbovenzi requested changes Apr 19, 2022
View reviewed changes
Copy link
Contributor

@bbovenzi bbovenzi left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/confirm, /failed and /success work. But /clear is not processing multiple map_index params chained together.

The UI branch at mapped-instance-actions is up-to-date if you want to manually test it.

@uranusjr
Copy link
Member

Because I forgot to modify /clear to expect multiple map_index params 🤦‍♂️

@uranusjr uranusjr force-pushed the clearing-and-marking-mapped-task branch from 0a52609 to a890173 Compare April 20, 2022 02:40
@uranusjr uranusjr force-pushed the clearing-and-marking-mapped-task branch from a890173 to b1b2fd1 Compare April 20, 2022 13:53
bbovenzi
bbovenzi approved these changes Apr 20, 2022
View reviewed changes
Copy link
Contributor

@bbovenzi bbovenzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything is working as expected from the UI now

ephraimbuddy and others added 13 commits April 20, 2022 17:41
@ephraimbuddy @ashb
Allow marking/clearing mapped taskinstances from the UI
44d58f4
@ephraimbuddy @ashb
fixup! Allow marking/clearing mapped taskinstances from the UI
7c63b5c
@ephraimbuddy @ashb
fixup! fixup! Allow marking/clearing mapped taskinstances from the UI
c149958
@ephraimbuddy @ashb
fixup! fixup! fixup! Allow marking/clearing mapped taskinstances from…
cc099fa 
… the UI
@ephraimbuddy @ashb
add tests
d4cb610
@ephraimbuddy @ashb
fixup! fixup! fixup! fixup! Allow marking/clearing mapped taskinstanc…
d0d0a41 
…es from the UI
@ephraimbuddy @ashb
fixup! fixup! fixup! fixup! fixup! Allow marking/clearing mapped task…
0c30cee 
…instances from the UI
@ephraimbuddy @ashb
fixup! fixup! fixup! fixup! fixup! fixup! Allow marking/clearing mapp…
14ad8f7 
…ed taskinstances from the UI
@ephraimbuddy @jedcunningham
@uranusjr @ashb
Apply suggestions from code review
6c3b250 
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
@ephraimbuddy @ashb
fixup! Apply suggestions from code review
78f91bc
@uranusjr @ashb
Refactor to straighten up types
4b4f1dc
@uranusjr @ashb
Accept multiple map_index param from front end
2e8a878 
This allows setting multiple instances of the same task to SUCCESS or
FAILED in one request. This is translated to multiple task specifier
tuples (task_id, map_index) when passed to set_state().

Also made some drive-through improvements adding types and clean some
formatting up.
@uranusjr @ashb
Introduce tuple_().in_() shim for MSSQL compat
19ba72a
@ashb ashb force-pushed the clearing-and-marking-mapped-task branch from b1b2fd1 to 19ba72a Compare April 20, 2022 16:41
@ashb ashb merged commit 4fa718e into apache:main Apr 20, 2022
@jedcunningham jedcunningham added changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) area:dynamic-task-mapping AIP-42 labels Apr 25, 2022
@jedcunningham jedcunningham added this to the Airflow 2.3.0 milestone Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jedcunningham jedcunningham jedcunningham left review comments

@uranusjr uranusjr uranusjr approved these changes

@bbovenzi bbovenzi bbovenzi approved these changes

@ryanahamilton ryanahamilton Awaiting requested review from ryanahamilton ryanahamilton is a code owner

@ashb ashb Awaiting requested review from ashb ashb is a code owner

@mik-laj mik-laj Awaiting requested review from mik-laj

@kaxil kaxil Awaiting requested review from kaxil kaxil is a code owner

@XD-DENG XD-DENG Awaiting requested review from XD-DENG XD-DENG is a code owner

Assignees
No one assigned
Labels
area:API Airflow's REST/HTTP API area:dynamic-task-mapping AIP-42 area:webserver Webserver related Issues changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Milestone
Airflow 2.3.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ephraimbuddy @bbovenzi @uranusjr @jedcunningham @ashb