-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Alias task_display_name for EventLogResponse
#55160
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
Alias task_display_name for EventLogResponse
#55160
Conversation
660d08e to
5a2d4f4
Compare
09e7e2a to
3c90f03
Compare
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! Thanks for the PR!
pierrejeambrun
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 good to me. We can merge once other comments are resolved.
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.
LGTM as well
|
Feel free to resolve comments when they are address so we can merge it @guan404ming (unless there is more work planned here) |
Sure, let me resolve them. It's currently looks good for me but if TP still have any questions please feel free to let me know. I could help follow up or clarify. Thanks! |
7889406 to
c1dadfb
Compare
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.
That makes me realize (I might be wrong) that we are not eagerly loading that relationship on the get_event_logs endpoint, which means that we are lazily emitting many request when serializing responses right?
We probably want to fix that by adding loading options? (I think we have the same issue for retrieving the dag_display_name, dagmodel will be joined lazyly)
4617540 to
ec87997
Compare
ec87997 to
81c45f5
Compare
* Alias task_display_name for event_logs * Update lazy loading to noload
* Alias task_display_name for event_logs * Update lazy loading to noload
* Alias task_display_name for event_logs * Update lazy loading to noload
* Alias task_display_name for event_logs * Update lazy loading to noload
* Alias task_display_name for event_logs * Update lazy loading to noload
* Alias task_display_name for event_logs * Update lazy loading to noload
* Alias task_display_name for event_logs * Update lazy loading to noload
* Alias task_display_name for event_logs * Update lazy loading to noload
* Alias task_display_name for event_logs * Update lazy loading to noload
* Alias task_display_name for event_logs * Update lazy loading to noload
* Alias task_display_name for event_logs * Update lazy loading to noload
* Alias task_display_name for event_logs * Update lazy loading to noload
* Alias task_display_name for event_logs * Update lazy loading to noload
* Alias task_display_name for event_logs * Update lazy loading to noload
* Alias task_display_name for event_logs * Update lazy loading to noload
* Alias task_display_name for event_logs * Update lazy loading to noload
* Alias task_display_name for event_logs * Update lazy loading to noload
* Alias task_display_name for event_logs * Update lazy loading to noload (cherry picked from commit 78b8b3c)
|
Manual backport #57609 |
Related Issue
#46730
How
task_display_nameforEventLogResponse^ 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.