Skip to content

Conversation

@guan404ming
Copy link
Member

Related Issue

closes #41140
cc @eladkal

Why

  • prevents infinite polling when monitored failure
  • correctly propagates failure status back to calling workflow

How

  • fix WorkflowTrigger to continue checking allowed_states when no failed states are found
  • update ExternalTaskSensor.execute_complete to properly handle "failed" status
  • add tests to verify behaviors

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

Nice

@eladkal eladkal requested review from Lee-W and kaxil May 12, 2025 13:07
@Lee-W
Copy link
Member

Lee-W commented May 12, 2025

Looks like we might need to fix some compat issue 🤔

@guan404ming guan404ming force-pushed the fix-external-task branch 2 times, most recently from 1e14d3f to 399ef6e Compare May 12, 2025 15:08
@guan404ming
Copy link
Member Author

Pushed for adding test for exception.

@guan404ming
Copy link
Member Author

guan404ming commented May 12, 2025

Mark it draft for fixing failing ci and would reopen when it is ready.

@guan404ming guan404ming marked this pull request as draft May 12, 2025 18:09
@guan404ming guan404ming force-pushed the fix-external-task branch 3 times, most recently from e7c3048 to d407b15 Compare May 13, 2025 05:20
@guan404ming
Copy link
Member Author

CI failure seems fixed in #50521, reopen it for review.

@guan404ming guan404ming marked this pull request as ready for review May 13, 2025 06:39
@eladkal eladkal force-pushed the fix-external-task branch from 0cad112 to 91af5b4 Compare May 13, 2025 08:06
@eladkal eladkal requested a review from Lee-W May 13, 2025 08:07
Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

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

It looks fantastic! Thank you for clarifying the exceptions so much.

@eladkal
Copy link
Contributor

eladkal commented May 13, 2025

static checks fails

Co-authored-by: Wei Lee <weilee.rx@gmail.com>
@guan404ming
Copy link
Member Author

I've fixed it.

@Lee-W Lee-W merged commit 0da1106 into apache:main May 13, 2025
95 checks passed
@guan404ming
Copy link
Member Author

Thanks for all reviews and suggestions!

if failed_count > 0:
yield TriggerEvent({"status": "failed"})
return
else:
Copy link
Member

Choose a reason for hiding this comment

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

I am confused this why this else removed?
What if there is no allowed_states provided and only provided failed_states? i believe it goes infinite loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing out.

The else statement was removed to ensure that even if no tasks are found in a failed state (failed_count == 0), the trigger proceeds to check the allowed_states. Previously, it would immediately yield a "success" event and exit, bypassing the allowed_states check.

But in the case you've mentioned it would get into infinite loop, originally I've considered that case too. But I think "success" in this context to "the specified failure condition was not met." not really "success". But it do cause issue for users to get into the infinite loop

To address the continuous polling when only failed_states are specified and no failure occurs, maybe we could add this check below to prevent get into the loop. wdyt?

elif not self.allowed_states and not self.skipped_states:
      yield TriggerEvent({"status": "success"}) # Or a new status like "no_failure_detected"
      return

sanederchik pushed a commit to sanederchik/airflow that referenced this pull request Jun 7, 2025
* Update `external_task` to forward failed_stat

* Apply suggestions from code review

Co-authored-by: Wei Lee <weilee.rx@gmail.com>

* Fix AF2 test

---------

Co-authored-by: Wei Lee <weilee.rx@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ExternalTaskSensor deferrable doesn't fail with failed_states

4 participants