ramitkataria commented on code in PR #73568:
URL: https://github.com/apache/airflow/pull/73568#discussion_r4077656523
##########
airflow-core/src/airflow/utils/db_cleanup.py:
##########
@@ -484,14 +484,23 @@ def _do_delete(
# found. The SELECT includes the same NOT EXISTS guard, so the
skipped row is
# excluded on the next pass too and the loop drains naturally.
With --batch-size
# set, continuing lets subsequent batches clean rows unaffected by
the race.
- if deleted == 0:
- logger.warning(
- "Some rows from %s are still referenced by another table
and were not "
- "deleted; they remain in %s and will be retried on the
next cleanup run.",
- source_table_name,
- target_table_name if not skip_archive else "the archive
(which is being dropped)",
- )
- continue
+ #
+ # Compare against the archive rather than testing ``deleted ==
0``: the archive
+ # holds exactly the rows this pass found, so any shortfall is a
skipped row. A
+ # partial skip is the likely case and is also the harmful one,
because the
+ # archive has already committed a copy of a row that is still live.
+ if skip_if_referenced:
+ archived =
session.scalars(select(func.count()).select_from(target_table)).one()
+ if deleted < archived:
+ logger.warning(
+ "%s of %s rows from %s are still referenced by another
table and were "
+ "not deleted; they remain in %s and will be retried on
the next cleanup run.",
Review Comment:
Nit: don't the skipped rows remain in `dag_version` rather than the archive?
With `skip_archive=True` this reads as "they remain in the archive (which is
being dropped)", which sounds like the surviving rows are about to go away.
Maybe name the source table here and mention separately that the archive still
holds a copy?
##########
airflow-core/src/airflow/utils/db_cleanup.py:
##########
@@ -484,14 +484,23 @@ def _do_delete(
# found. The SELECT includes the same NOT EXISTS guard, so the
skipped row is
# excluded on the next pass too and the loop drains naturally.
With --batch-size
# set, continuing lets subsequent batches clean rows unaffected by
the race.
- if deleted == 0:
- logger.warning(
- "Some rows from %s are still referenced by another table
and were not "
- "deleted; they remain in %s and will be retried on the
next cleanup run.",
- source_table_name,
- target_table_name if not skip_archive else "the archive
(which is being dropped)",
- )
- continue
+ #
+ # Compare against the archive rather than testing ``deleted ==
0``: the archive
+ # holds exactly the rows this pass found, so any shortfall is a
skipped row. A
+ # partial skip is the likely case and is also the harmful one,
because the
+ # archive has already committed a copy of a row that is still live.
+ if skip_if_referenced:
+ archived =
session.scalars(select(func.count()).select_from(target_table)).one()
+ if deleted < archived:
+ logger.warning(
+ "%s of %s rows from %s are still referenced by another
table and were "
+ "not deleted; they remain in %s and will be retried on
the next cleanup run.",
+ archived - deleted,
+ archived,
+ source_table_name,
+ target_table_name if not skip_archive else "the
archive (which is being dropped)",
+ )
+ continue
Review Comment:
It seems like this `continue` is not doing anything?
--
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]
