seanghaeli commented on code in PR #75708:
URL: https://github.com/apache/airflow/pull/75708#discussion_r4485497883
##########
airflow-core/src/airflow/serialization/definitions/dag.py:
##########
@@ -836,6 +837,15 @@ def _process_dagrun_deadline_alerts(
)
stats.incr("deadline_alerts.deadline_creation_failed",
tags=metrics_tags)
+ # An alert can roll the session back, discarding the pending
DagRun. Carrying on would
+ # add Deadlines that violate the dagrun_id foreign key, and
returning normally would let
+ # the caller treat the missing run as created.
+ if orm_dagrun not in session:
Review Comment:
My agent flagged this one: only `_create_dag_runs` catches the new error.
The asset-triggered path (`_create_dag_runs_asset_triggered`) and the
partitioned asset path (`_create_dagruns_for_partitioned_asset_dags`) call
`create_dagrun` with no try/except, so for an asset-scheduled Dag the
`DagRunRolledBackError` goes up through `_do_scheduling` and the scheduler
exits. Before this change that case just quietly skipped the run, so this would
be a new crash. I checked it in Breeze with an asset-scheduled Dag whose alert
rolls back the session, and `_create_dag_runs_asset_triggered` raises.
Could you handle it in those two paths too, and add a test with an
asset-scheduled Dag? The run failing visibly instead of being created is fine
by me, since the rollback can't be undone from in there.
--
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]
