Calculate retry eligibility before task runs #47996
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ashb
reviewed
Mar 20, 2025
ashb
approved these changes
Mar 20, 2025
Member
ashb
left a comment
ashb
left a comment
There was a problem hiding this comment.
Question about naming aside, LGTM.
Some simple tests worth adding though I think
kaxil
approved these changes
Mar 20, 2025
amoghrajesh
reviewed
Mar 20, 2025
uranusjr
force-pushed
the
check-retry-eligibility-ahead-of-time
branch
from
180e20a to
f34454d
Compare
Member
Author
|
I made some changes to how the runner sets state and how the API handles it. The FAIL_WITHOUT_RETRY state has been removed; now FAILED always means FAILED, and the runner may send UP_TO_RETRY for the API to handle. Tests are to come; I want to see CI first to decide what to add. |
uranusjr
force-pushed
the
check-retry-eligibility-ahead-of-time
branch
from
f34454d to
042133a
Compare
amoghrajesh
reviewed
Mar 21, 2025
uranusjr
force-pushed
the
check-retry-eligibility-ahead-of-time
branch
2 times, most recently
from
d3e40e0 to
868e7fe
Compare
uranusjr
force-pushed
the
check-retry-eligibility-ahead-of-time
branch
from
868e7fe to
10f3170
Compare
The task needs this information to know whether it should run the retry or failed callback if the task fails, so we calculate it eagerly and pass it to the runner, instead of lazily only after the task has failed. The information is used by the task runner to decide what state to set the ti to when an error happens. If the task is eligible for retry, the next state is set to UP_FOR_RETRY; otherwise it's FAILED. This is then handled accordingly in the API. The FAIL_WITHOUT_RETRY state has been removed from the code base since it is no longer needed. The state was introduced in 1283cc3 to tell the API server to not check retry eligibility on certain failure cases, but now that eligibility is always checked and used in the task runner, the API server no longer needs this information; a request to update to FAILED always set the TI to FAILED.
uranusjr
force-pushed
the
check-retry-eligibility-ahead-of-time
branch
from
10f3170 to
93155b3
Compare
agupta01
pushed a commit
to agupta01/airflow
that referenced
this pull request
Mar 21, 2025
The task needs this information to know whether it should run the retry or failed callback if the task fails, so we calculate it eagerly and pass it to the runner, instead of lazily only after the task has failed. The information is used by the task runner to decide what state to set the ti to when an error happens. If the task is eligible for retry, the next state is set to UP_FOR_RETRY; otherwise it's FAILED. This is then handled accordingly in the API. The FAIL_WITHOUT_RETRY state has been removed from the code base since it is no longer needed. The state was introduced in 1283cc3 to tell the API server to not check retry eligibility on certain failure cases, but now that eligibility is always checked and used in the task runner, the API server no longer needs this information; a request to update to FAILED always set the TI to FAILED.
shubham-pyc
pushed a commit
to shubham-pyc/airflow
that referenced
this pull request
Mar 22, 2025
The task needs this information to know whether it should run the retry or failed callback if the task fails, so we calculate it eagerly and pass it to the runner, instead of lazily only after the task has failed. The information is used by the task runner to decide what state to set the ti to when an error happens. If the task is eligible for retry, the next state is set to UP_FOR_RETRY; otherwise it's FAILED. This is then handled accordingly in the API. The FAIL_WITHOUT_RETRY state has been removed from the code base since it is no longer needed. The state was introduced in 1283cc3 to tell the API server to not check retry eligibility on certain failure cases, but now that eligibility is always checked and used in the task runner, the API server no longer needs this information; a request to update to FAILED always set the TI to FAILED.
nailo2c
pushed a commit
to nailo2c/airflow
that referenced
this pull request
Apr 4, 2025
The task needs this information to know whether it should run the retry or failed callback if the task fails, so we calculate it eagerly and pass it to the runner, instead of lazily only after the task has failed. The information is used by the task runner to decide what state to set the ti to when an error happens. If the task is eligible for retry, the next state is set to UP_FOR_RETRY; otherwise it's FAILED. This is then handled accordingly in the API. The FAIL_WITHOUT_RETRY state has been removed from the code base since it is no longer needed. The state was introduced in 1283cc3 to tell the API server to not check retry eligibility on certain failure cases, but now that eligibility is always checked and used in the task runner, the API server no longer needs this information; a request to update to FAILED always set the TI to FAILED.
kaxil
added a commit
to astronomer/airflow
that referenced
this pull request
Apr 8, 2025
This was broken in apache#47996 closes apache#48927
kaxil
added a commit
that referenced
this pull request
Apr 8, 2025
kosteev
pushed a commit
to GoogleCloudPlatform/composer-airflow
that referenced
this pull request
May 29, 2025
This was broken in apache/airflow#47996 closes apache/airflow#48927 GitOrigin-RevId: d5ea5890fd7a8b769935638c50a46214a553fc44
kosteev
pushed a commit
to GoogleCloudPlatform/composer-airflow
that referenced
this pull request
Sep 24, 2025
This was broken in apache/airflow#47996 closes apache/airflow#48927 GitOrigin-RevId: d5ea5890fd7a8b769935638c50a46214a553fc44
kosteev
pushed a commit
to GoogleCloudPlatform/composer-airflow
that referenced
this pull request
Oct 22, 2025
This was broken in apache/airflow#47996 closes apache/airflow#48927 GitOrigin-RevId: d5ea5890fd7a8b769935638c50a46214a553fc44
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The task needs this information to know whether it should run the retry or failed callback if the task fails, so we calculate it eagerly and pass it to the runner, instead of lazily only after the task has failed.
When a task fails, it uses this information to decide whether it should go directly into FAIL_WITHOUT_RETRY, so the server does not need to calculate this again.This makes the TITerminalState a bit wonky. Now the FAILED state actually makes the server always retry the task, and should probably be renamed to RETRY or something...? And FAIL_WITHOUT_RETRY can actually just be called FAILED since there's no other kind of failed. A simple rename causes a lot of things to change though since the enum is reused in a lot of places. So I decided to leave the names be for now.The information is used by the task runner to decide what state to set the ti to when an error happens. If the task is eligible for retry, the next state is set to UP_FOR_RETRY; otherwise it's FAILED. This is then handled accordingly in the API.
The FAIL_WITHOUT_RETRY state has been removed from the code base since it is no longer needed. The state was introduced in #45106 to tell the API server to not check retry eligibility on certain failure cases, but now that eligibility is always checked and used in the task runner, the API server no longer needs this information; a request to update to FAILED always set the TI to FAILED.