-
Notifications
You must be signed in to change notification settings - Fork 15.9k
Added last_asset_event_id and last_asset_event_timestamp to the AssetResponse
#50060
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
|
@pierrejeambrun, @bbovenzi, #49994 closed in favor of this PR. |
…f max(AssetEvent.id) has the max(AssetEvent.timestamp)
bbovenzi
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.
Manually testing in the UI and lgtm.
@pierrejeambrun can you take a look for the API portion? We'll want to add sorting and filtering soon.
bugraoz93
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. I only have a comment related to a loop in the code :) Thanks for the PR!
airflow-core/src/airflow/api_fastapi/core_api/routes/public/assets.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/core_api/routes/public/assets.py
Outdated
Show resolved
Hide resolved
bugraoz93
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.
Thanks for the update! Looks good
|
Changed implemented! |
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.
Just wondering if last_asset_event_id and last_asset_event_timestamp shouldn't be flat in the asset model but in a nested object such as:
"last_asset_event": {
"id":
"timestamp":
}
(A lighter version of the AssetEventResponse)
That's a minor detail but if we later need more fields from the last asset event, then we will have all those field flats mixed up with the asset model.
I think that makes total sense. I'll go ahead and implement that this morning. |
|
@pierrejeambrun, I've gone ahead and implemented that change! |
airflow-core/src/airflow/api_fastapi/core_api/routes/public/assets.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/core_api/routes/public/assets.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/core_api/datamodels/assets.py
Outdated
Show resolved
Hide resolved
rawwar
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.
Thanks! Looking good. Just a few nits
…s.py Co-authored-by: Kalyan R <[email protected]>
…sets.py Co-authored-by: Kalyan R <[email protected]>
…sets.py Co-authored-by: Kalyan R <[email protected]>
|
Unrelated CI failure, merging. |
|
Thanks @jroachgolf84! |
This pull request adds the
last_asset_event_idandlast_asset_event_timestampfields to theAssetResponsemodel, and thus, the/assetsendpoint. The original use-case was to make the Asset list view sortable by the last Asset Event. However, there is a much larger use-case at play here; the ability to easily determine the timestamp for the last Asset Event for a certain Asset. This is especially applicable for Asset Watchers.By default, the
last_asset_event_idandlast_asset_event_timestampfields are returned asnull. It's not until the first Asset Event that these fields are populated.Unit-tests were added/updated for all changes made.
related: #47164