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.

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

Add new type of exception to catch timeout #42064

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

VladaZakharova
Copy link
Contributor

With old logic every ImportError that was raised by Timeout of reading the DAG was not stored in database and as a result, was not exposed to UI.
This PR adds additional Exception type to be catch, that will ensure that AirflowTaskTimeout exception will be also caught and stored in DB as other normal Import errors.

Closes #41976


^ 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 newsfragments.

@potiuk potiuk added this to the Airflow 2.10.2 milestone Sep 6, 2024
@potiuk potiuk merged commit abe5f7c into apache:main Sep 6, 2024
51 checks passed
@potiuk
Copy link
Member

potiuk commented Sep 6, 2024

Thanks @VladaZakharova !

potiuk pushed a commit to potiuk/airflow that referenced this pull request Sep 6, 2024
RNHTTR pushed a commit that referenced this pull request Sep 6, 2024
(cherry picked from commit abe5f7c)

Co-authored-by: VladaZakharova <uladaz@google.com>
@kosteev
Copy link
Contributor

kosteev commented Sep 12, 2024

I am pretty sure it works, just to double confirm.

Briefly looking the code it looks like AirflowTaskTimeout will be raised in context manager "timeout" outside of "parse" method:

with timeout(dagbag_import_timeout, error_message=timeout_msg):

Will be AirflowTaskTimeout be catched really by try/except in the "parse" method?

Also, would be good to have unit test for this. To prevent regression and also avoid e.g. doubts that I raise in this question:)

@VladaZakharova @potiuk

@VladaZakharova
Copy link
Contributor Author

Hi!
Yes, good question, thank you for pointing this out. According to my tests this exception is indeed caught and on UI we can see the output of the actual error of Timeout:
Screenshot 2024-09-12 10 14 50

I will add a unit test for this in a separate PR
Thank you!

ephraimbuddy pushed a commit that referenced this pull request Sep 13, 2024
(cherry picked from commit abe5f7c)

Co-authored-by: VladaZakharova <uladaz@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DAG import timeouts recorded in Airflow Database and displayed in Airflow UI
3 participants