Warn for bad combination of parameters for RedshiftDataOperator

[amoghrajesh]

What

RedshiftDataOperator has two independent parameters, deferrable and wait_for_completion. Since #41206, wait_for_completion=False intentionally overrides deferrable=True and if a task isnt waiting for the statement to finish at all, there is nothing to defer. That is the correct behavior, but it happens silently: a user who sets deferrable=True gets no signal that it was ignored.

Current behaviour

Combining deferrable=True with wait_for_completion=False submits the statement and returns immediately, exactly as if deferrable=False had been set but with no warning, log line, or error indicating that the deferrable setting had no effect.

Proposed change

Added a warning in RedshiftDataOperator that fires when this combination is detected, telling the user both whats set and the resulting behavior:

A log.warning rather than a raised exception was a deliberate choice cos this is long standing, already released behavior (not a new feature being gated), so raising would break any existing DAG relying on this combination for zero functional benefit. The goal is discoverability, not enforcement.

---

Was generative AI tooling used to co-author this PR?

• Yes (please specify the tool below)

---

• Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
• For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
• When adding dependency, check compliance with the ASF 3rd Party License Policy.
• For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

[jason810496]

LGTM for this one, but wonder how could we address this common regression, thanks.

[eladkal]

I am not in the details here but the description sounds like a generic problem rather than a single operator?

[amoghrajesh]

@eladkal thanks for your comment, i went through a bit and this isn't Redshift specific. I checked the rest of providers/amazon for the same shape (deferrable silently defeated by another flag, no warning) and found it in 6 more operators across 2 files:

• dms.py: DmsModifyTaskOperator, DmsDeleteReplicationConfigOperator, DmsStartReplicationOperator, DmsStopReplicationOperator
• redshift_cluster.py: RedshiftResumeClusterOperator, RedshiftPauseClusterOperator, these are actually worse, since wait_for_completion defaults to False there, so deferrable=True is a no-op out of the box, not just in an edge-case combination.

Given the scope, I suggest keeping this PR focused on RedshiftDataOperator and tracking the other 6 operators as separate follow-up work rather than expanding this PR to touch unrelated operators. Happy to open a tracking issue for that if useful.

[eladkal]

@eladkal thanks for your comment, i went through a bit and this isn't Redshift specific. I checked the rest of providers/amazon for the same shape (deferrable silently defeated by another flag, no warning) and found it in 6 more operators across 2 files:

• dms.py: DmsModifyTaskOperator, DmsDeleteReplicationConfigOperator, DmsStartReplicationOperator, DmsStopReplicationOperator
• redshift_cluster.py: RedshiftResumeClusterOperator, RedshiftPauseClusterOperator, these are actually worse, since wait_for_completion defaults to False there, so deferrable=True is a no-op out of the box, not just in an edge-case combination.

Given the scope, I suggest keeping this PR focused on RedshiftDataOperator and tracking the other 6 operators as separate follow-up work rather than expanding this PR to touch unrelated operators. Happy to open a tracking issue for that if useful.

That is OK
My thoughts were more if we should have it handled by base class? That is the better way to avoid such cases in the future?
I am also wondering if there may be similar pitfalls in other providers.
Probably need @o-nikolas @vincbeck guiance here to what they think is best for AWS

[jason810496]

My thoughts were more if we should have it handled by base class? That is the better way to avoid such cases in the future?
I am also wondering if there may be similar pitfalls in other providers.

Same thought for this.

[amoghrajesh]

I think I will wait for Vincent / Niko's reply here till I merge this one.

[vincbeck]

Good topic, this inconsistency across operators has always bothered me. I am on the side of:

• wait_for_completion configures whether you want to wait (regardless of the mode)
• deferrable configures whether you want to use the triggerer to wait or the worker (if wait_for_completion is True)

So I agree wait_for_completion=False and deferrable=True seems to not making a lot of sense, BUT, actually I think it is, what happens if an environment set AIRFLOW__OPERATORS__DEFAULT_DEFERRABLE to True? Every operator with wait_for_completion=False would receive a warning? I also know that some operators decide to wait solely based on the value of deferrable which I think is wrong, because again, if AIRFLOW__OPERATORS__DEFAULT_DEFERRABLE is set to True on the environment level, it makes all operators wait.

WDYT?

[ferruzzi]

I think at some point the prevailing thought was "if you set deferrable=True then of course that implies you do not want to wait", which is also valid. But I think the main conflict just stems from the fact that deferrable was added after wait and maybe not everything was standardized over when deferrables were implemented.

[amoghrajesh]

Good point @vincbeck, and you arre right that its ambiguous as written. self.deferrable here cannot tell the difference between "the user wrote deferrable=True" and "the environment's default_deferrable config resolved it to True", both look identical by the time __init__ runs.

I thought about fixing that distinction (e.g. defaulting deferrable to None and only warning when it was explicitly passed), but that pattern deferrable: bool = conf.getboolean("operators", "default_deferrable", fallback=False) is used identically across ~100 other operators just in this provider. Making RedshiftDataOperator behave differently from all of them felt like a bigger, more invasive change than this PR should make just to add a courtesy warning.

So I plan to rather keep this scoped to what it is: a heads up for the common case of someone writing this contradiction directly. If an environment sets default_deferrable=True globally and also uses wait_for_completion=False a lot, yes, they'll see this warning but I'd like to argue that is not entirely wrong either: it is accurate information that their environment wide default is a no-op for those tasks.

[vincbeck]

Good point @vincbeck, and you arre right that its ambiguous as written. self.deferrable here cannot tell the difference between "the user wrote deferrable=True" and "the environment's default_deferrable config resolved it to True", both look identical by the time __init__ runs.

I thought about fixing that distinction (e.g. defaulting deferrable to None and only warning when it was explicitly passed), but that pattern deferrable: bool = conf.getboolean("operators", "default_deferrable", fallback=False) is used identically across ~100 other operators just in this provider. Making RedshiftDataOperator behave differently from all of them felt like a bigger, more invasive change than this PR should make just to add a courtesy warning.

So I plan to rather keep this scoped to what it is: a heads up for the common case of someone writing this contradiction directly. If an environment sets default_deferrable=True globally and also uses wait_for_completion=False a lot, yes, they'll see this warning but I'd like to argue that is not entirely wrong either: it is accurate information that their environment wide default is a no-op for those tasks.

Looks valid to me

[amoghrajesh]

Thanks @vincbeck! Let me see what best I can do of it.