sadpandajoe commented on code in PR #45029:
URL: https://github.com/apache/superset/pull/45029#discussion_r4203141767
##########
docs/docs/using-superset/recently-archived.mdx:
##########
@@ -159,6 +159,20 @@ destructive when it runs. Set it to `True` in
`superset_config.py` and restart
the relevant processes to log what the task *would* purge without deleting
anything. Validate the eligible backlog before enabling live purging or
shortening a window; the first live run can process accumulated old archives.
+`SOFT_DELETE_PURGE_MAX_PER_RUN` defaults to `1000` root charts, dashboards, or
+datasets per scheduled run. One root's complete cascade counts as one and is
+never split to fit the cap. Set `0` or `None` in `superset_config.py` to remove
+the cap. It bounds committed deletions, not candidate evaluations: blocked
roots
+are re-evaluated every run, so a blocked-heavy backlog can still require
+substantial work. A negative, boolean, string, or fractional value stops the
+scheduled task without purging. A live run reports `cap_reached` and
`remaining_eligible`
Review Comment:
This says a live run reports `cap_reached` and `remaining_eligible`, right
after telling operators they can set `0` or `None` to remove the cap. In
unlimited mode `_add_purge_cap_stats` returns early, so none of `cap_reached`,
`remaining_eligible`, `remaining_count_complete` (or the matching gauges) are
emitted. An operator who switches to unlimited and keeps alerting on those
fields would see them vanish and may read that as a measurement failure. Should
this sentence say the fields appear only when a positive cap is configured?
##########
tests/unit_tests/tasks/test_version_history_retention.py:
##########
@@ -407,9 +409,260 @@ def test_immediate_cutoff_and_invalid_skip(stats:
MagicMock, days: int) -> None:
days
)
if days == -1:
- run_pass.assert_called_once_with(now, [], 0)
+ run_pass.assert_called_once_with(now, [], 0, max_prune=None)
assert result["cutoff"] == now.isoformat()
else:
assert result == {"skipped": 1}
tables.assert_not_called()
run_pass.assert_not_called()
+
+
+def test_prune_cap_accounts_only_for_committed_passes(stats: MagicMock) ->
None:
+ """A retried pass uses the same allowance and cannot overshoot the cap."""
+ tables: version_history_retention.ShadowTables = (
+ version_history_retention.ShadowTables(
+ parent=[], child=[], m2m=None, transaction=MagicMock()
+ )
+ )
+ first: dict[str, int] = {
+ "candidate_count": 1000,
+ "max_candidate_id": 1000,
+ "pruned_transactions": 2,
+ }
+ second: dict[str, int] = {
+ "candidate_count": 1000,
+ "max_candidate_id": 2000,
+ "pruned_transactions": 1,
+ }
+ run_pass: MagicMock
+ with (
+ patch.object(
+ version_history_retention, "_resolve_shadow_tables",
return_value=tables
+ ),
+ patch.object(
+ version_history_retention,
+ "_run_pass_with_retry",
+ side_effect=[(first, 1), (second, 0)],
+ ) as run_pass,
+ patch.object(
+ version_history_retention, "_probe_prunable", return_value=(4,
False)
+ ),
+ ):
+ result: dict[str, Any] =
version_history_retention._prune_old_versions_impl(
+ retention_days=30, max_per_run=3
+ )
+
+ assert result["pruned_transactions"] == 3
+ assert result["retried"] == 1
+ assert result["cap_reached"] is True
+ assert result["remaining_eligible"] == 4
+ assert result["remaining_count_complete"] is False
+ stats.gauge.assert_any_call(
+ "superset.versioning.retention.remaining_count_complete", 0
+ )
+ assert run_pass.call_args_list[0].kwargs["max_prune"] == 3
+ assert run_pass.call_args_list[1].kwargs["max_prune"] == 1
+
+
[email protected]("cap", [None, 0, 3])
+def test_prune_dry_run_counts_all_eligible_without_writes(
+ stats: MagicMock, cap: int | None
+) -> None:
+ """A dry run reports the whole backlog and capped-run estimate."""
+ tables: version_history_retention.ShadowTables = (
+ version_history_retention.ShadowTables(
+ parent=[], child=[], m2m=None, transaction=MagicMock()
+ )
+ )
+ run_pass: MagicMock
+ with (
+ patch.object(
+ version_history_retention, "_resolve_shadow_tables",
return_value=tables
+ ),
+ patch.object(version_history_retention, "_count_prunable",
return_value=7),
+ patch.object(version_history_retention, "_run_pass_with_retry") as
run_pass,
+ ):
+ result: dict[str, Any] =
version_history_retention._prune_old_versions_impl(
+ 30, max_per_run=cap, dry_run=True
+ )
+
+ assert result["eligible_backlog"] == 7
+ assert result["estimated_capped_runs"] == (3 if cap == 3 else 1)
+ run_pass.assert_not_called()
+ stats.gauge.assert_not_called()
+
+
[email protected]("helper_name", ["_count_prunable", "_probe_prunable"])
+def test_remainder_reads_use_default_isolation(helper_name: str) -> None:
+ """Backlog measurement does not request the delete pass's isolation."""
+ tables: version_history_retention.ShadowTables = (
+ version_history_retention.ShadowTables(
+ parent=[], child=[], m2m=None, transaction=MagicMock()
+ )
+ )
+ engine: MagicMock = MagicMock()
+ window: version_history_retention._PruneWindow = (
+ version_history_retention._PruneWindow(
+ prunable=[], candidate_count=0, max_candidate_id=0
+ )
+ )
+ with (
+ patch.object(version_history_retention, "db") as mock_db,
+ patch.object(
+ version_history_retention, "_resolve_prune_window",
return_value=window
+ ),
+ ):
+ mock_db.engine = engine
+ result: int | tuple[int, bool] = getattr(
+ version_history_retention, helper_name
+ )(datetime(2026, 1, 1), tables)
+
+ assert result in (0, (0, True))
+ engine.connect.return_value.execution_options.assert_not_called()
+
+
+def test_dry_run_count_closes_read_transaction_between_windows() -> None:
+ """A full backlog scan releases each read snapshot before the next
window."""
+ tables: version_history_retention.ShadowTables = (
+ version_history_retention.ShadowTables(
+ parent=[], child=[], m2m=None, transaction=MagicMock()
+ )
+ )
+ engine: MagicMock = MagicMock()
+ full_window: version_history_retention._PruneWindow = (
+ version_history_retention._PruneWindow(
+ prunable=[],
Review Comment:
Both windows here have `prunable=[]` and the mocked `_resolve_prune_window`
ignores its `after_id` argument, so this only proves the connection is closed
between windows. If `_count_prunable` reset `total` on each window, or stopped
advancing `after_id` to `max_candidate_id`, the test would still pass, and
`eligible_backlog` / `estimated_capped_runs` in dry-run would silently
undercount a multi-window backlog. Could you give the first window a couple of
prunable IDs and the final window one more, then assert the total is their sum
and that the second `_resolve_prune_window` call receives the first window's
`max_candidate_id`?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
