seanghaeli commented on code in PR #53000:
URL: https://github.com/apache/airflow/pull/53000#discussion_r2196295182
##########
airflow-core/src/airflow/utils/state.py:
##########
@@ -213,7 +213,7 @@ def color_fg(cls, state):
"""
success_states: frozenset[TaskInstanceState] = frozenset(
- [TaskInstanceState.SUCCESS, TaskInstanceState.SKIPPED]
+ [TaskInstanceState.SUCCESS, TaskInstanceState.SKIPPED,
TaskInstanceState.REMOVED]
Review Comment:
1. Implications for `TerminalStateNonSuccess`:
- In the main branch, I've verified that when calling the `finish`
function in `client.py` with state parameter as `UPSTREAM_FAILED`, an error
will be thrown. This is due to the following line of the function:
https://github.com/apache/airflow/blob/ac9e968fe85cf43717d280f303470eb5268ca9ca/task-sdk/src/airflow/sdk/api/client.py#L157
In which `TerminalStateNonSuccess("upstream_failed")` is problematic
My conclusion then is that nowhere in the main branch is using the
`finish` function in this way, otherwise we would be seeing errors.
- In this PR, looking into a future situation where the `finish`
function is called with the state parameter as `UPSTREAM_FAILED`, the
`test_task_instance_finish` test verifies that it works:
https://github.com/apache/airflow/blob/59fa23fd2215354576efeefad77b506449eb2c36/task-sdk/tests/task_sdk/api/test_client.py#L287-L308
- I've spun up a DAG where there is a task with terminal state
`SUCCESS`, and another task with terminal state `UPSTREAM_FAILED`. For both
tasks, the `finish` function is not being called (I put an exception within the
function and it doesn't get triggered, the state change is being triggered from
somewhere else). Additionally, both in the main branch and in this PR, the
`end_date` associated with the Task Instance with `UPSTREAM_FAILED` gets set
normally, so I don't think there's issues with that specifically.
2. Adding `REMOVED` to the `success_states` set:
- This addresses the part of the PR that says `failed + success =
terminal`. Currently, without this change, the union of the `success` and
`failed` sets does not cover all of the `terminal` states.
- I can't reproduce a situation where a Task gets put into the `REMOVED`
state since when that happens the DAG version just updates and the missing Task
just disappears on the Airflow UI. If you agree that `REMOVED` is not a
relevant terminal state, then I can go ahead and remove it from the list of
success states and we can still effectively consider all reachable `failed +
success` states indeed equal `terminal`.
--
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]
