seanmuth commented on PR #73142:
URL: https://github.com/apache/airflow/pull/73142#issuecomment-5836790821
Addressed the three optional nits from the approval review, and rebased over
#73249/#73253 in the process, worth a second look on the latter given your
approval predates those merges.
**The three nits:**
- The SUCCESS bullet now correctly scopes the 409 claim to
`should_retry=False`; with retries enabled, the pre-fix code resolves to
`UP_FOR_RETRY` (in `STATES_SENT_DIRECTLY`), so no write happens and no 409
follows, but the state is still wrong (UP_FOR_RETRY instead of SUCCESS). Also
reworded "reported via message" to cover SERVER_TERMINATED, which the
supervisor sets directly with no subprocess message at all.
- The test comments ("genuinely observed, e.g. from a SIGTERM kill") were
wrong for a positive exit code -- signal deaths are negative. Corrected to cite
the actually-reachable positive-exit-code path: an exception raised inside
`finalize()`'s callbacks (`task_runner.py`'s outer `except Exception ...
sys.exit(1)`).
- The PR body now mentions the third case: `TaskState(FAILED)` with
`should_retry=True`, reachable via `task_runner.py` regardless of retry
eligibility, where the pre-fix code loses the FAILED report to a spurious
`UP_FOR_RETRY`.
**The rebase:** #73249 restructured how `TaskState`-reported outcomes get
dispatched -- they now go through `update_task_state_if_needed()`'s
pending-message path using the message's own state directly, never consulting
`final_state` while a message is pending. That makes the SKIPPED-case fix here
defense-in-depth rather than the reachable bug it was when you reviewed it; the
SUCCESS case remains fully live and reachable either way. #73253 also added its
own narrower precedence special-case for `SERVER_TERMINATED`/`UP_FOR_RETRY`
directly in `final_state`; the general `_terminal_state is not None` check here
subsumes both under one rule, so I removed the narrower tuple rather than
keeping both. Full diff against current `main` is now just the `final_state`
property itself, nothing else touched.
294 passed (up from 211 pre-rebase, picking up #73249/#73253's own new
tests), 1 skipped (platform-only), mypy clean.
---
Drafted-by: Claude Sonnet 5 (no human review before posting)
--
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]
