Add async asset store accessors for async tasks and watcher triggers - #72127
vatsrahul1001 merged 3 commits into
Conversation
|
@jason810496 - this is great, this is something I had on my radar when I originally ported in the Accessor for use with |
|
This was my original PR: #68900 |
|
@jason810496, I think we'd need to allow for the async methods to be used from the Triggerer? Similar to what I did in my PR above? cc: @amoghrajesh |
Yes, but they can be done in the follow-up, since there're only two triggers using the Asset Store so far. |
There was a problem hiding this comment.
Just head up that we need to get this one in recently as the downstream Iceberg provider change just merged.
Additionally, I just realized we need a compatible layer for the aget method on provider side, see #72173 (review) for more context.
amoghrajesh
left a comment
There was a problem hiding this comment.
LGTM some nits mostly.
jason810496
left a comment
There was a problem hiding this comment.
Thanks for the review.
Backport failed to create: v3-3-test. View the failure log Run detailsNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
You can attempt to backport this manually by running: cherry_picker 2aa581e v3-3-testThis should apply the commit to the v3-3-test branch and leave the commit in conflict state marking After you have resolved the conflicts, you can continue the backport process by running: cherry_picker --continueIf you don't have cherry-picker installed, see the installation guide. |
|
I'm going to follow up with the PR to allow these to be used on the Triggerer. |
…pache#72127) * Add async asset store accessors for async tasks and watcher triggers * Clarify async asset state accessor docstrings * Parametrize async asset state accessor tests
…pache#72127) * Add async asset store accessors for async tasks and watcher triggers * Clarify async asset state accessor docstrings * Parametrize async asset state accessor tests
Why
While reviewing #71135 and #71387, I realized that we should support the async methods for
AssetStateStoreAccessorfollowing the pattern ofTaskStateStoreAccessorintroduced in #68232.How
_build_*_messagehelper plus a shared_extract_get_response, then have the sync methodsend()and the new async methodawait asend()the same message. No behaviour change on the sync path.adelete/aclearawaitBaseStoreBackend.adelete/aclear, which already exist on the backend base class, and keep the DB-ref-first ordering of their sync counterparts.AssetStateStoreAccessorsfacade. That is the objectTriggerRunnerinjects astrigger_instance.asset_state_store, so it is what watcher triggers actually call.Known gaps (inherited from #68232)
BaseStoreBackendhas no asyncserialize/deserializevariant, soaget/asetstill call the sync ref (de)serialization on the event-loop thread. With a real-I/O worker backend that can stall the loop despite the async API. Tracked in Async state store accessors block the event loop on sync ref (de)serialization #72129.adelete/acleardelete the DB ref first, then awaitbackend.adelete/aclear, whichproviders/common/ioraisedNotImplementedError. Follow-up in Implement async state store methods for object storage backend #72131.Was generative AI tooling used to co-author this PR?