Add Kinesis Data Streams trigger - #71135
Conversation
|
Those failures seem to be caused by the same issue: There is already a PR to fix it: #71118 |
4e1733b to
d7e4df5
Compare
|
Waiting for another PR #71145 to be merged. |
d7e4df5 to
5589794
Compare
|
Okay, all CI failures have been fixed. |
|
Hi @jason810496, I'd appreciate it if you could take a look when you have time. :) |
|
Thanks for the reviews, I believe this version is more concrete than before :) |
jason810496
left a comment
There was a problem hiding this comment.
Thanks for adding the new trigger to compelete #52712.
| def _save_checkpoint(self, sequence_numbers: dict[str, str]) -> None: | ||
| store = getattr(self, "asset_state_store", None) |
| try: | ||
| store.set(self._build_checkpoint_key(), dict(sequence_numbers)) | ||
| except ValueError: |
There was a problem hiding this comment.
This is the second PR that touched the Task Store within the trigger. #71387 is the first one.
cc @amoghrajesh to double check regarding the usage.
Just in case using Task Store within trigger isn't an expected pattern.
There was a problem hiding this comment.
It's a good pattern to use (but slight correction: its asset state store not task state store), he Triggerer itself injects it at: https://github.com/apache/airflow/blob/main/airflow-core/src/airflow/jobs/triggerer_job_runner.py#L1405-L1408, so a BaseEventTrigger attached to a watched asset is designed to read and write asset state. Watermarking a stream cursor is one of the use case.
| @property | ||
| def hook(self) -> KinesisHook: |
There was a problem hiding this comment.
Not sure do we need to define as cached property or not? We might need to check the lifecycle of the Kinesis client. IIRC, this might be related to the https://lists.apache.org/thread/owp7869xmr5cyks9gx3lp66x53w1owyg discussion ( #71701 PR).
Just in case Kinesis client invoke eager external calls during construction.
| config=self.botocore_config, | ||
| ) | ||
|
|
||
| def _build_checkpoint_key(self) -> str: |
There was a problem hiding this comment.
Would it be better to make this as a cache property? (e.g. asset_store_checkpoint_key
amoghrajesh
left a comment
There was a problem hiding this comment.
Blocking due to sync access to asset state store
| async def _get_records( | ||
| self, | ||
| client: BaseAwsConnection, | ||
| shard_id: str, | ||
| shard_iterator: str, | ||
| after_sequence_number: str | None, | ||
| fallback_iterator_type: str, | ||
| ) -> dict[str, Any]: | ||
| try: | ||
| return await client.get_records( | ||
| ShardIterator=shard_iterator, | ||
| Limit=self.batch_size, | ||
| ) | ||
| except client.exceptions.ExpiredIteratorException: | ||
| shard_iterator = await self._get_shard_iterator( | ||
| client, | ||
| shard_id, | ||
| after_sequence_number, | ||
| fallback_iterator_type, | ||
| ) | ||
| return await client.get_records( | ||
| ShardIterator=shard_iterator, | ||
| Limit=self.batch_size, | ||
| ) |
There was a problem hiding this comment.
A second ExpiredIteratorException will propagate through and kill the trigger.
| :param shard_iterator_type: Position used when a shard has no checkpoint. ``LATEST`` only sees records | ||
| that arrive after the watcher starts; ``TRIM_HORIZON`` starts from the oldest retained record. |
There was a problem hiding this comment.
AT_TIMESTAMP and AT_SEQUENCE_NUMBER are rejected?
related: #52712
This is the first PR for Kinesis part of #52712, I will integrate the trigger with the common-messaging interface and add a system/e2e test in the next PR.
Summary
Add an asynchronous Kinesis Data Streams trigger for consuming records without occupying worker slots.
The trigger polls all shards, emits JSON-serializable record batches, handles iterator expiry, throttling, pagination, and in-run resharding, and checkpoints per-shard sequence numbers when an asset state store is available. New shards first discovered after a restart honor the configured initial position, so
LATESTmay skip records written during the downtime.This is the provider-side foundation for the common-messaging integration tracked in #52712.
Was generative AI tooling used to co-author this PR?
Generated-by: [GPT 5.6-sol] following the guidelines