ferruzzi commented on code in PR #74113:
URL: https://github.com/apache/airflow/pull/74113#discussion_r4342621250
##########
airflow-core/src/airflow/utils/db_cleanup.py:
##########
@@ -605,44 +615,51 @@ def _do_delete(
)
except BaseException:
- error_raised = True
- # Roll back the failed transaction so its locks are released before
- # the archive table is dropped in the ``finally`` block below.
- # ``rollback()`` itself can raise (e.g. the connection died);
suppress
- # it so it does not shadow the original error being re-raised.
with suppress(Exception):
session.rollback()
raise
- finally:
- if target_table is not None and skip_archive:
- # Drop the archive table on the session's own connection.
Binding
- # the drop to ``session.get_bind()`` (the Engine) would check
out a
- # *second* pooled connection, and on MySQL its ``DROP TABLE``
blocks
- # indefinitely on the metadata lock still held by this
session's
- # open transaction when the DELETE above failed -- the ``db
clean``
- # hang reported in #66177.
- try:
- target_table.drop(bind=session.connection())
- session.commit()
- except Exception:
- # If we are already unwinding from a delete failure, a
cleanup
- # error here must not replace the original exception
(Python
- # makes a ``finally``-raised error the top-level one). Log
and
- # let the original delete error keep propagating. On the
success
- # path (no delete error), a drop/commit failure is a real
- # problem, so re-raise it.
- if not error_raised:
- raise
- logger.warning(
- "Failed to drop archive table %s while cleaning up
after a "
- "delete failure; propagating the original delete error
instead.",
- target_table_name,
- exc_info=True,
- )
print("Finished Performing Delete")
+def _delete_directly(
Review Comment:
It would likely be more efficient to build the table once and pass it into
this method, no? Maybe I'm missing a reason to redo it each pass like this?
##########
airflow-core/src/airflow/utils/db_cleanup.py:
##########
@@ -507,6 +507,21 @@ def _do_delete(
else:
print("Performing Delete...")
+ if skip_archive:
+ try:
+ _delete_directly(
+ limited_query=limited_query,
+ source_table_name=source_table_name,
+ session=session,
+ dialect_name=dialect_name,
+ )
+ session.commit()
+ except BaseException:
+ with suppress(Exception):
+ session.rollback()
+ raise
+ continue
Review Comment:
As written, this continue is right for the fix but will cause another
because it makes the whole skip_if_referenced block around L613 unreachable.
One fix would be to have `_delete_directly` return the number of rows
deleted and compare that against a count of the selection, warning when it
deleted fewer than it found. `deleted == 0` won't do, since L597-600 rejects it
for staying silent on a partial skip. You can throw the warning at L604 into a
helper that both paths can call.
--
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]
