-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Support TaskInstanceHistory in log handlers #51592
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
Support TaskInstanceHistory in log handlers #51592
Conversation
3a151e7 to
67b7226
Compare
dstandish
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.
looks ok to me
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.
Related to #50175
The #50175 try to fix on FastAPI side instead of FileTaskHandler, but introduce other error ( #50175 (comment) ).
6d22132 to
e92a2a7
Compare
I have tested for the error mentioned in the issue. It works fine with this change. |
e92a2a7 to
4d624db
Compare
4d624db to
d0a94c3
Compare
jedcunningham
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.
Overall, looks good. Are other task handlers impacted by these typing changes? If so, probably can be addressed in a follow up as they'd be in providers...
IMO, we don't need to adapt TIH type hit for other provider-based task handlers, as most of them already adapted to RemoteIO interface ( which use |
I can do a follow-up PR if required. |
Support TaskInstanceHistory in log handlers
Support TaskInstanceHistory in log handlers
Support TaskInstanceHistory in log handlers (cherry picked from commit 5749e12)
This change fixes the log API to fully support
TaskInstanceHistoryby adding anidalias, aget_dagrun()helper toTaskInstanceHistory. This also updatesFileTaskHandlerto recognise history objects, and fixes the missingtry_number filterin the FastAPI route so that retry task logs can be fetched correctly.I have tested it by updating
log_filename_template = dag_id={{ ti.id }}/run_id={{ ti.run_id }}/task_id={{ ti.task_id }}/{%% if ti.map_index >= 0 %%}map_index={{ ti.map_index }}/{%% endif %%}attempt={{ try_number|default(ti.try_number) }}.logand ran the task, then cleared the task instance and checked the log.closes: #51525
^ 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.