Re: [PR] Fix airflow db clean never purging the callback table [airflow]

["ferruzzi (via GitHub)" <gi...@apache.org>]

ferruzzi commented on code in PR #70923:
URL: https://github.com/apache/airflow/pull/70923#discussion_r3973492699


##########
airflow-core/src/airflow/utils/db_cleanup.py:
##########
@@ -205,7 +219,22 @@ def readable_config(self):
     _TableConfig(table_name="task_reschedule", recency_column_name="start_date", dag_id_column_name="dag_id"),
     _TableConfig(table_name="xcom", recency_column_name="timestamp", dag_id_column_name="dag_id"),
     _TableConfig(table_name="_xcom_archive", recency_column_name="timestamp", dag_id_column_name="dag_id"),
-    _TableConfig(table_name="callback_request", recency_column_name="created_at"),
+    _TableConfig(
+        table_name="callback",
+        recency_column_name="created_at",
+        extra_columns=["state"],

Review Comment:
   I'm surprised this doesn't need `id` in the `extra_columns`, it should be a required column.



##########
airflow-core/src/airflow/utils/db_cleanup.py:
##########
@@ -205,7 +219,22 @@ def readable_config(self):
     _TableConfig(table_name="task_reschedule", recency_column_name="start_date", dag_id_column_name="dag_id"),
     _TableConfig(table_name="xcom", recency_column_name="timestamp", dag_id_column_name="dag_id"),
     _TableConfig(table_name="_xcom_archive", recency_column_name="timestamp", dag_id_column_name="dag_id"),
-    _TableConfig(table_name="callback_request", recency_column_name="created_at"),
+    _TableConfig(
+        table_name="callback",
+        recency_column_name="created_at",
+        extra_columns=["state"],
+        # A callback that can still run owns its deadline row through an ON DELETE CASCADE
+        # foreign key, so purging one would silently drop a deadline that has not fired yet.
+        # Dag-processor callbacks carry no state and are deleted as they are dispatched; any
+        # that outlive the retention window were orphaned and no deadline references them.
+        extra_filters=[
+            or_(
+                column("state").in_(sorted(TERMINAL_STATES)),

Review Comment:
   This may be backwards, isn't it?  Don't we want to check if `column("state").not_in(sorted(ACTIVE_STATES)) OR column("state").is_(None),`?  If a deadline is in SCHEDULED and its run is deleted, then it's orphaned as well and needs to be pruned, no?  It would also mean renaming or adding new states later become pruned by default which would have prevented this issue.



-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org