pierrejeambrun commented on PR #62812:
URL: https://github.com/apache/airflow/pull/62812#issuecomment-4281454132
### Concerns
1. **Most tests mock `set_task_group_state`** —
`test_patch_task_group_success`, `_failed_state`, `_nested`,
`_includes_upstream_downstream_parameters`,
`_dry_run_returns_affected_tis_without_committing` all mock the core logic.
This means the tests verify the route/service glue but NOT that the
v2.x-equivalent logic in `SerializedDAG.set_task_group_state` is correct. The
query-count tests (`test_query_count_does_not_scale_with_task_group_size`,
`test_dry_run_query_count_does_not_scale`) and
`test_patch_task_group_updates_ti_states_in_db` don't mock and are the real
regression guards — keep those, but add more unmocked tests for:
- `include_upstream=True` / `include_downstream=True` — verify actual
upstream/downstream TIs get their state set
- `past=True` / `future=True` — verify runs before/after the target are
affected AND the `exclude_run_ids` logic works
- Empty or no-op group (409 path)
- The `clear()` of failed downstream tasks actually resumes them
2. **`_collect_unique_tis` may be unnecessary** — the key `(dag_id, run_id,
task_id, map_index)` isn't necessary if `set_task_group_state` already returns
unique TIs (the underlying `set_state` dedupes). Only the `"note"` branch adds
the tis separately, but those are all within the group too — so the dedup
helper is defensive rather than necessary. Could be simplified.
3. **`_patch_ti_group_validate_request` is near-duplicate of
`_patch_ti_validate_request`** — differs only in replacing
`task_id`/`map_index` with `task_group_id` and calling
`_get_task_group_task_instances`. Consider parameterizing or extracting the
common validation logic.
4. **`_get_task_group_task_instances` uses `.join(TI.dag_run)` with no
DagRun filter** — Copilot flagged this too. Since `TI.dag_id == dag_id,
TI.run_id == run_id` already constrain the result, the join is redundant.
Remove or justify with a comment (e.g., "inner join enforces the DagRun
exists"). Also `joinedload(TI.rendered_task_instance_fields)` is applied here
AND again in the route (`patch_task_group_instances` does a re-query with
`populate_existing=True`). Looks like duplicated eager-load work — verify the
first one is actually used, otherwise drop it.
5. **Conflict on "already in target state"** (`_patch_task_group_state`)
```python
if not updated_tis:
raise HTTPException(status.HTTP_409_CONFLICT, ...)
```
The single-TI endpoint returns an empty list instead of 409 (I argued
against 404 earlier). Inconsistent behavior between "single TI already in
state" (empty 200) and "all group TIs already in state" (409). Align them:
return empty list 200.
6. **`dag.set_task_group_state` in the service is called with `commit=True`,
but the route function also calls it via `_patch_task_group_state` which also
defaults `commit=True`**. Not a bug, but if someone ever calls
`_patch_task_group_state` expecting `commit=False`, there's no way to override
— the helper hardcodes `commit=True`. Consider threading `commit` as a
parameter for future reuse.
7. **`_patch_ti_group_validate_request` uses `Query(None)`** for
`update_mask` — copilot flagged this. `Query` is a FastAPI dependency marker;
using it in a service helper is misleading (the param isn't actually populated
from the request here since this is called internally). Use `update_mask:
list[str] | None = None`.
9. **OpenAPI spec regenerated but not verified** —
`v2-rest-api-generated.yaml` has 162 additions. Review the spec diff for
correct request/response schemas, especially the 409 response added per point 5
above.
--
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]
