-
Notifications
You must be signed in to change notification settings - Fork 16k
Better error handling for running ti heartbeats after task is cleared #56443
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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 <[email protected]>
…_instances.py Co-authored-by: Amogh Desai <[email protected]>
…_instances.py Co-authored-by: Amogh Desai <[email protected]>
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is added here by mistake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting looks off -- static checks will fail
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.