Better error handling for running ti heartbeats after task is cleared#56443
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py
Outdated
Show resolved
Hide resolved
…_instances.py Co-authored-by: Amogh Desai <amoghrajesh1999@gmail.com>
…_instances.py Co-authored-by: Amogh Desai <amoghrajesh1999@gmail.com>
…_instances.py Co-authored-by: Amogh Desai <amoghrajesh1999@gmail.com>
| status_code=status.HTTP_204_NO_CONTENT, | ||
| responses={ | ||
| status.HTTP_404_NOT_FOUND: {"description": "Task Instance not found"}, | ||
| status.HTTP_410_GONE: {"description": "Task Instance no longer exists, it may have moved to the Task Instance History table"}, |
There was a problem hiding this comment.
I think 404 should still exist, there’s a difference between a ti that never existed and a ti that existed but is gone.
amoghrajesh
left a comment
There was a problem hiding this comment.
I think these changes are insufficient, look at my comments
| ) | ||
| except NoResultFound: | ||
| log.error("Task Instance not found") | ||
| status.HTTP_410_GONE: {"description": "Task Instance no longer exists, it may have moved to the Task Instance History table"}, |
There was a problem hiding this comment.
This is added here by mistake?
There was a problem hiding this comment.
I think we should actually validate that, check if it is present in the TIH table.
Two situations possible here:
- Task Instance doesnt exist and not in TIH table -> return 404
- Task Instance doesnt exist and present in TIH table -> return 410
| detail={ | ||
| "reason": "not_found", | ||
| "message": "Task Instance not found", | ||
| status.HTTP_410_GONE: {"description": "Task Instance no longer exists, it may have moved to the Task Instance History table"}, |
There was a problem hiding this comment.
Formatting looks off -- static checks will fail
|
@kriti-sc hello, did you get a chance to look at the comments on this one? |
Return
410 - GONEinstead of404 - NOT_FOUNDwhen the running task instance heartbeats after task is cleared.When a running task is cleared, the previous task instance is moved from the Task Instance table to the Task Instance History table. As a result, if the task instance hearbeats, the api server returns a 404. A more appropriate error is 410.
closes: #53140
Airflow task logs:
Scheduler and API server logs:
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.