Skip to content
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

AIP-72: Handling failed TI state for AirflowTaskTerminated #44977

Merged

Conversation

amoghrajesh
Copy link
Contributor

@amoghrajesh amoghrajesh commented Dec 17, 2024

related: #44414

This is an addition to #44954 and is dependent on it.
Only last commit is relevant cb493ee

We do not have to handle the case of external task state updates specifically in task_runner because public UI APIs will directly do that job - core APIs. So there is no scenario when one of these exceptions are thrown due to this:

except ServerResponseError as e:
if e.response.status_code in {HTTPStatus.NOT_FOUND, HTTPStatus.CONFLICT}:
log.error(
"Server indicated the task shouldn't be running anymore",
detail=e.detail,
status_code=e.response.status_code,
)
self.kill(signal.SIGTERM, force=True)

We check with the database for the state with the heartbeat and if ever there is a different state, other than running:

if previous_state != State.RUNNING:
raise HTTPException(
status_code=status.HTTP_409_CONFLICT,
detail={
"reason": "not_running",
"message": "TI is no longer in the running state and task should terminate",
"current_state": previous_state,
},
)
, we fail with a 409.

But, if this state is ever hit, we should just mark the TI as failed. There are UTs https://github.com/apache/airflow/compare/main...astronomer:airflow:AIP72-taskterminated-or-afexception?expand=1#diff-413c3c59636a3c7b41b8bb822827d18a959778d0b6331532e0db175c829dbfd2R342-R397 which handle this portion to check if it fails or not.


^ 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.rst or {issue_number}.significant.rst, in newsfragments.

@amoghrajesh amoghrajesh added area:task-execution-interface-aip72 AIP-72: Task Execution Interface (TEI) aka Task SDK area:task-sdk labels Dec 17, 2024
@amoghrajesh amoghrajesh changed the title AIP-72: Handling failed TI state for AirflowException & AirflowTaskTerminated AIP-72: Handling failed TI state for AirflowException & AirflowTaskTerminated Dec 17, 2024
@amoghrajesh amoghrajesh changed the title AIP-72: Handling failed TI state for AirflowException & AirflowTaskTerminated AIP-72: Handling failed TI state for AirflowTaskTerminated Dec 17, 2024
@amoghrajesh amoghrajesh requested review from ashb and kaxil December 17, 2024 16:19
@amoghrajesh amoghrajesh self-assigned this Dec 18, 2024
@amoghrajesh amoghrajesh merged commit 65b110d into apache:main Dec 19, 2024
48 checks passed
@amoghrajesh amoghrajesh deleted the AIP72-taskterminated-or-afexception branch December 19, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:task-execution-interface-aip72 AIP-72: Task Execution Interface (TEI) aka Task SDK area:task-sdk
Development

Successfully merging this pull request may close these issues.

2 participants