nathadfield commented on code in PR #60052:
URL: https://github.com/apache/airflow/pull/60052#discussion_r2660786075
##########
providers/standard/src/airflow/providers/standard/operators/trigger_dagrun.py:
##########
@@ -193,7 +193,8 @@ def __init__(
self.skip_when_already_exists = skip_when_already_exists
self.fail_when_dag_is_paused = fail_when_dag_is_paused
self.openlineage_inject_parent_info = openlineage_inject_parent_info
- self.deferrable = deferrable
+ # deferrable only makes sense when wait_for_completion=True
+ self.deferrable = deferrable and wait_for_completion
Review Comment:
@shahar1 The problem is that users can have existing DAGs with
`deferrable=True` and `wait_for_completion=False` (often due to this options
being set by `default_args`). These tasks now get stuck in `DEFERRED` state -
this is the bug I was attempting to fix.
Raising an exception would prevent future misconfigurations (good), but it
would break existing DAGs that have this invalid configuration. For users who
set `deferrable=True` at a DAG/cluster policy level in their config, every
TriggerDagRunOperator instance would need explicit `deferrable=False` (since
`wait_for_completion` defaults to `False`). Since these DAGs are already broken
(tasks stuck), we're choosing to fix them rather than fail them.
The Trade-off:
- Your approach: Explicit validation, but breaking change requiring
explicit `deferrable=False`
- My approach: Silent fix, non-breaking, but less explicit
Would a deprecation warning (now) → exception (next major version) be
acceptable? This fixes existing broken DAGs while still moving toward explicit
validation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
