kaxil commented on code in PR #73889:
URL: https://github.com/apache/airflow/pull/73889#discussion_r4250716954
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/task_instances.py:
##########
@@ -94,7 +94,7 @@ class TaskInstanceResponse(BaseModel):
default=None,
validation_alias="retry_reason",
description=(
- "The reason the task instance reached its current state, as
recorded by a retry policy. May describe a previous attempt: it is cleared only
when the task next starts running, so a task waiting to be retried or re-run
can still carry the reason its last attempt ended."
+ "The reason the task instance reached its current state, as
recorded by a retry policy. Describes the attempt that is currently recorded on
the row: it is cleared when the task is cleared and when the next attempt is
scheduled or deferred."
Review Comment:
This sentence doesn't hold while the task is up_for_retry. The retry route
writes the reason and then `prepare_db_for_next_try()` bumps `try_number`, so
the row shows try N+1 with try N's reason, and `next_retry_datetime()` depends
on that. The old caveat that a task waiting to be retried still carries the
reason its last attempt ended is still true and worth keeping; only the
"cleared when it next starts running" part needed to change.
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/task_instance_history.py:
##########
@@ -68,7 +68,7 @@ class TaskInstanceHistoryResponse(BaseModel):
default=None,
validation_alias="retry_reason",
description=(
- "The reason the task instance reached its current state, as
recorded by a retry policy. May describe a previous attempt: it is cleared only
when the task next starts running, so a task waiting to be retried or re-run
can still carry the reason its last attempt ended."
+ "The reason the task instance reached its current state, as
recorded by a retry policy. Describes the attempt that is currently recorded on
the row: it is cleared when the task is cleared and when the next attempt is
scheduled or deferred."
Review Comment:
This model mostly serializes archived tries, and nothing clears
`task_instance_history.retry_reason`. The new history check in
`test_clear_task_instances_clears_retry_policy_columns` asserts the archived
reason survives the clear. Could the history model get its own description,
something like "The reason this try ended, as recorded by a retry policy"?
##########
airflow-core/tests/unit/models/test_dagrun.py:
##########
@@ -2949,6 +2955,44 @@ def execute_complete(self):
assert ti.state == TaskInstanceState.DEFERRED
[email protected]_serialized_dag
+def test_schedule_tis_start_trigger_clears_retry_policy_columns(dag_maker,
session):
Review Comment:
This copies `TestOperator` and the setup from
`test_schedule_tis_start_trigger` above. Parametrizing that test over `None` /
`UP_FOR_RETRY` with the two columns seeded would cover it the same way
`test_schedule_tis_preserves_allocated_attempt` does now.
##########
airflow-core/src/airflow/models/taskinstance.py:
##########
@@ -447,6 +447,10 @@ def clear_task_instances(
ti.max_tries = max(ti.max_tries, previous_try_number)
ti.state = None
ti.external_executor_id = None
+ # retry_delay_override is the functional one:
next_retry_datetime() prefers it over
+ # the task's own retry_delay, so a stale value would retime the
next retry.
+ ti.retry_reason = None
Review Comment:
One path this doesn't reach: marking an up_for_retry TI as success, failed
or skipped goes through `TaskInstance.set_state()`, which leaves both columns,
so the terminal try N+1 shows try N's reason. If it's cleared later,
`prepare_db_for_next_try()` at line 424 archives try N+1 with that reason
before this reset runs. Should `set_state` clear them when leaving
up_for_retry, or would you rather take that in a follow-up?
##########
airflow-core/src/airflow/models/dagrun.py:
##########
@@ -2296,6 +2296,9 @@ def schedule_tis(
state=TaskInstanceState.SCHEDULED,
scheduled_dttm=timezone.utcnow(),
try_number=next_try_number,
+ # Already archived with the finished try; the new one
must not inherit it.
+ retry_reason=None,
+ retry_delay_override=None,
Review Comment:
The EmptyOperator UPDATE further down (the `empty_ti_ids` block around line
2347) uses the same state clause and `next_try_number` but doesn't reset these
two. A task swapped to a trivial EmptyOperator while its TI waits in
up_for_retry lands in SUCCESS on try N+1 with try N's reason. Adding the same
two `None`s there keeps both attempt-allocating UPDATEs consistent.
--
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]
