jedcunningham commented on code in PR #72100:
URL: https://github.com/apache/airflow/pull/72100#discussion_r3865680488


##########
airflow-core/newsfragments/72100.significant.rst:
##########


Review Comment:
   I'll let @vatsrahul1001 chime in, but imo we don't need a significant note 
for this one.



##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/task_instances.py:
##########
@@ -986,6 +987,12 @@ def _collect_relatives(run_id: str, direction: 
Literal["upstream", "downstream"]
         except AirflowClearRunningTaskException as e:
             raise HTTPException(status.HTTP_409_CONFLICT, str(e)) from e
 
+        # Clearing means "run this again", so the next attempt starts over 
rather than resuming from
+        # progress recorded by the attempt the user just discarded. Only after 
the clear has
+        # succeeded, so a failed clear cannot take the task state with it.

Review Comment:
   Probably don't need the front end of this comment, pretty intuitive. Could 
keep the "only after clear has suceeded" part, still optional imo.



##########
airflow-core/src/airflow/api_fastapi/core_api/services/public/task_instances.py:
##########
@@ -74,21 +80,29 @@ def _clear_task_state_store_on_success(tis: Sequence[TI], 
session: Session) -> N
         try:
             backend.clear(scope=scope, session=session)
             log.info(
-                "Cleared task state on success",
+                event,
                 dag_id=ti.dag_id,
                 run_id=ti.run_id,
                 task_id=ti.task_id,
                 map_index=ti.map_index,
             )
         except Exception:
             log.warning(
-                "Failed to clear task state on success",
+                "Failed to discard task state",
+                event=event,
                 dag_id=ti.dag_id,
                 run_id=ti.run_id,
                 task_id=ti.task_id,
             )
 
 
+def _clear_task_state_store_on_success(tis: Sequence[TI], session: Session) -> 
None:
+    """Discard task state store entries for each TI if clear_on_success is 
enabled."""
+    if not conf.getboolean("state_store", "clear_on_success", fallback=False):

Review Comment:
   ```suggestion
       if not conf.getboolean("state_store", "clear_on_success"):
   ```
   
   Don't need the fallback, defaults come from config already.



-- 
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]

Reply via email to