Add deferrable mode to QuickSight operator and sensor - #70218
Conversation
o-nikolas
left a comment
There was a problem hiding this comment.
Looks like there are conflicts and build failures that need looking at. Also, I'm not sure of the breaking change (the new 30min timeout). I think large ingestions will easily pass that and fail dags for folks using this operator. We could make the default much higher, or go through a deprecation process where we log that the user should switch to setting a high default before we remove it. Or wait until a major release for the amazon provider package.
| **kwargs, | ||
| ): | ||
| super().__init__(**kwargs) | ||
| if check_interval is not None: |
There was a problem hiding this comment.
This is kind of weird, you set a default value and then you check whether a value is assigned to it. In other words, anyone using this operator without setting check_interval will get a deprecation warning. I would leave the default value to None and keep following the deprecation warning process
There was a problem hiding this comment.
The default is None, so the warning only fires when check_interval is passed
explicitly. It's a few lines above the context collapsed here:
Happy to change it if you meant something else.
Raised the default wait to 36 hours: 4320 attempts at the 30s delay, or 2160 at the sensor's 60s poke interval. That's how long AWS itself waits on a SPICE ingestion in the CloudFormation wait policy, where 36 hours is both the default and the maximum: |
The QuickSight create-ingestion wait path used a hand-written polling loop that held a worker slot with time.sleep and had no upper bound, so a stuck ingestion could block a worker indefinitely. Moving the wait to a custom boto waiter lets both the operator and the sensor defer to the triggerer with deferrable=True, matching the deferrable pattern used across the Amazon provider, and bounds the synchronous wait as well.
AWS waits up to 36 hours for a SPICE ingestion in its own CloudFormation wait policy, where 36 hours is both the default and the maximum. Refreshes that run for several hours are common, so a shorter cap fails Dags that previously waited without a limit. https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-quicksight-dataset-ingestionwaitpolicy.html
ca7307e to
5e254c3
Compare
Adds
deferrable=Truesupport toQuickSightCreateIngestionOperatorandQuickSightSensor, following the same waiter-based pattern as the rest of the Amazon provider. QuickSight has no official boto3 waiters (boto/boto3#2769), so this adds a customingestion_completewaiter and aQuickSightIngestionCompletedTrigger, and replaces the hook's hand-written polling loop, which held a worker slot withtime.sleepand had no upper bound.One thing without a direct precedent:
DescribeIngestionrequiresAwsAccountId, so it is resolved on the worker before deferring and passed to the trigger — the triggerer never makes a sync STS call.Behaviour changes: the synchronous wait is now bounded by
waiter_max_attempts(default 60 attempts × 30s) instead of waiting forever; noted in the provider changelog.QuickSightHook.wait_for_stateand the operator'scheck_intervalare deprecated (still working) in favour ofwait_for_ingestion/waiter_delay. Failures raiseQuickSightIngestionFailedError, anAirflowExceptionsubclass.Was generative AI tooling used to co-author this PR?