ashb commented on a change in pull request #16681:
URL: https://github.com/apache/airflow/pull/16681#discussion_r659976072
##########
File path: airflow/models/taskinstance.py
##########
@@ -1503,6 +1503,10 @@ def handle_failure_with_callback(
def is_eligible_to_retry(self):
"""Is task instance is eligible for retry"""
+ if self.state == State.CLEARED_WHEN_RUNNING:
+ # If a task was cleared when running, it is always eligible for
retry
Review comment:
I am not sure you'd is the case. If it's already run its maximal number
of tries out shouldn't run again should it?
##########
File path: airflow/models/taskinstance.py
##########
@@ -1503,6 +1503,10 @@ def handle_failure_with_callback(
def is_eligible_to_retry(self):
"""Is task instance is eligible for retry"""
+ if self.state == State.CLEARED_WHEN_RUNNING:
+ # If a task was cleared when running, it is always eligible for
retry
Review comment:
I am not sure ~you'd~ this is the case. If it's already run its maximal
number of tries out shouldn't run again should it?
##########
File path: airflow/models/taskinstance.py
##########
@@ -1503,6 +1503,10 @@ def handle_failure_with_callback(
def is_eligible_to_retry(self):
"""Is task instance is eligible for retry"""
+ if self.state == State.CLEARED_WHEN_RUNNING:
+ # If a task was cleared when running, it is always eligible for
retry
Review comment:
Okay yes, that is a compelling argument.
How about `RESTARTING` as the state name instead? It's slightly clearer what
the behaviour will be (to me at least)
Though I guess this isn't quite accurate either, as it would go back via the
scheduler loop, rather than a direct restart in the worker.)
Hmm
--
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]
