Skip to content

Conversation

@dzhigimont
Copy link
Contributor

Fix ExternalTaskSensor to work correctly with task groups that have mapped tasks.
Unit tests were added.
Closes: #30689

@dzhigimont dzhigimont force-pushed the fix_extarnal_task_sensor_zh branch 3 times, most recently from 1c669c9 to 52be4cb Compare April 21, 2023 14:37
@eladkal eladkal added this to the Airflow 2.6.1 milestone Apr 21, 2023
@eladkal eladkal added the type:bug-fix Changelog: Bug Fixes label Apr 21, 2023
@potiuk potiuk force-pushed the fix_extarnal_task_sensor_zh branch from 52be4cb to e51b053 Compare April 24, 2023 09:35
@dzhigimont
Copy link
Contributor Author

@uranusjr Could you please review?

@potiuk
Copy link
Member

potiuk commented Apr 29, 2023

LGTM. @uranusjr ?

@eladkal eladkal requested a review from uranusjr May 3, 2023 19:50
@dzhigimont dzhigimont force-pushed the fix_extarnal_task_sensor_zh branch from e51b053 to 2f22f02 Compare May 11, 2023 16:56
@dzhigimont
Copy link
Contributor Author

@uranusjr Ping, could you please take a look? 🙃

@uranusjr
Copy link
Member

This looks fine. I’m wondering though, instead of changing the logic to count (mapped) task instances, would it be easier to use something like GROUP BY to count tasks? In other words, if a task is mapped, we count 1 when and only when all the mapped instances of the task are finished, and 0 otherwise. This shouldn’t affect the general behaviour of the sensor, but I think can save at least one database query (if my mental model on this is correct).

@dzhigimont
Copy link
Contributor Author

This looks fine. I’m wondering though, instead of changing the logic to count (mapped) task instances, would it be easier to use something like GROUP BY to count tasks? In other words, if a task is mapped, we count 1 when and only when all the mapped instances of the task are finished, and 0 otherwise. This shouldn’t affect the general behaviour of the sensor, but I think can save at least one database query (if my mental model on this is correct).

@uranusjr Hi, I think it's not so easy to write that query, and it can be overcomplicated. For me, that overcomplication doesn't cost of saving one not heavy query.

@uranusjr
Copy link
Member

OK. I guess we could always optimise things, the proposed implementation is better than things being broken.

@potiuk potiuk merged commit 3c30e54 into apache:main May 18, 2023
eladkal pushed a commit that referenced this pull request Jun 8, 2023
* Fix ExternalTaskSensor to work correctly with task groups that have mapped tasks

* Add tests for ExternalTaskSensor with task group that have mapped tasks

* Fix working for multiple runs

* Use tuple_in_condition instead of tuple_

---------

Co-authored-by: Zhyhimont Dmitry <dzhigimont@gmail.com>
(cherry picked from commit 3c30e54)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug-fix Changelog: Bug Fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ExternalTaskSensor waits forever for TaskGroup containing mapped tasks

5 participants