Quantum0uasar commented on PR #59691:
URL: https://github.com/apache/airflow/pull/59691#issuecomment-4557172660
Reviewed the diff carefully. Great fix for a subtle race condition in
dynamic task group expansion. A few observations:
**On `_should_use_post_expansion_placeholder`:**
The function issues a DB query on every invocation inside what can be a hot
path (`get_relevant_upstream_map_indexes` is called for each upstream
dependency during scheduler evaluation). For DAGs with many mapped downstream
TIs this could add meaningful scheduler latency. Consider:
1. Caching the result keyed by `(run_id, task_id)` within the scope of a
single scheduling loop — the expansion state won't change mid-evaluation.
2. Alternatively, checking `ti.state` or `ti.expanded_map_indexes` fields
that are likely already loaded in the session identity map before falling back
to the DB query.
**On the condition logic:**
```python
return (
-1 in task_to_map_indexes[task.task_id]
and -1 not in task_to_map_indexes[relative.task_id]
and 0 in task_to_map_indexes[relative.task_id]
)
```
Is the third condition (`0 in task_to_map_indexes[relative.task_id]`)
strictly necessary? If `-1` is not in `relative`'s map indexes and expansion
has happened, `0` should always be present. The condition is correct but the
third check is implied by `not -1 in relative`. Worth a comment clarifying the
intent.
**On test coverage:**
The regression test covers `NONE_FAILED_MIN_ONE_SUCCESS` — would be good to
add a case for `ALL_SUCCESS` trigger rule too, since the upstream resolution
path is shared and the skip logic may differ.
Overall the approach is sound and the fix is minimal. Appreciate the
thorough rebasing history on this one.
--
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]
