-
Notifications
You must be signed in to change notification settings - Fork 16.3k
BUGFIX: Fixed timeout_after in run_trigger method of TriggerRunner #58282
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
Conversation
jason810496
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! However, it seems the CI still needs fix, thanks!
Yes I think I closed my laptop too fast, the commit with the fix wasn't included ;-) |
CI is fixed |
|
@thx Lee for merging this! |
|
Somehow it's not backported. Let me backport it manually |
…gerRunner (apache#58282) (cherry picked from commit 1c3ac6c) Co-authored-by: David Blain <info@dabla.be>
I discovered a bug in the run_trigger method of the TriggerRunner by accident due to the fact that I added typing to the trigger parameter when working on PR #51391.
There I got the following MyPy error from the Airflow CI/CD:
First I thought I did a mistake with the typing, but after closer investigation the typing was correct and so was also the MyPy error. Then I discovered that the timeout_after attribute was indeed never part of the BaseTrigger but was part of the workloads.RunTrigger model. So I refactored the run_trigger method so that beside trigger_id and trigger (now typed as BaseTrigger), now also has the optional timeout_after parameter which is passed from the workloads.RunTrigger model.
I don't know if this has ever lead to any issue at runtime, but I would certainly lead to an exception if a trigger would be cancelled.
^ 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.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.