kaxil commented on code in PR #69734:
URL: https://github.com/apache/airflow/pull/69734#discussion_r3624774558
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -677,6 +677,15 @@ def _create_ti_state_update_query_and_update_state(
if ti_patch_payload.trigger_timeout is not None:
timeout = timezone.utcnow() + ti_patch_payload.trigger_timeout
+ ti = session.get(TI, task_instance_id)
+ if ti is not None and ti.start_date is not None:
+ dag = dag_bag.get_dag_for_run(dag_run=ti.dag_run, session=session)
Review Comment:
This deserializes the full SerializedDAG on every deferral just to read one
task attribute (`execution_timeout`). `ti_update_state` with `state=deferred`
is a hot path -- every deferrable-sensor poke and every deferrable-operator
await hits it.
The sibling helper `_handle_fail_fast_for_dag` in this same file
deliberately avoids exactly this:
```python
# Check fail_fast from DagModel (simple column lookup) - early exit if False
# This avoids loading 5-50 MB SerializedDAG in 99% of cases
fail_fast = session.scalar(select(DagModel.fail_fast).where(DagModel.dag_id
== dag_id))
if not fail_fast:
return
# Only load SerializedDAG when fail_fast=True (rare case ~1%)
ser_dag = dag_bag.get_dag_for_run(dag_run=dr, session=session)
```
That guard only works because `fail_fast` is a `DagModel` column.
`execution_timeout` is task-level and lives only in the serialized blob, so
there's no cheap column to gate on -- every deferral here pays the full
deserialization unconditionally, even for the tasks (the majority) that set no
`execution_timeout`.
The worker already has the value with no DB hit: `_run_execute_callable`
reads `task.execution_timeout`, and the worker already ships
`trigger_timeout=defer.timeout` in the defer payload (`task_runner.py:1494` ->
`TIDeferredStatePayload.trigger_timeout`). Compute the execution deadline (or
the remaining budget) on the worker at defer time and send it in the payload
the way `trigger_timeout` already flows; the server then does `min(...)` with
zero deserialization.
Tradeoff: a new payload field needs an Execution API version bump (Cadwyn),
while the server-side load doesn't touch the wire. But a 5-50 MB
deserialization on every defer isn't worth saving a versioned field, and it
regresses the fail-fast optimization this file just added. The extra
`session.get(TI, ...)` full-ORM reload (the route only selected the columns it
needed) folds into the same fix.
--
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]
