EnxDev commented on code in PR #45029:
URL: https://github.com/apache/superset/pull/45029#discussion_r4200787034
##########
superset/tasks/version_history_retention.py:
##########
@@ -556,7 +683,23 @@ def prune_old_versions() -> dict[str, Any]:
retention_days: int = _resolve_version_history_retention_days(
configured, legacy_configured, seed=_version_history_retention_seed
)
- return _prune_old_versions_impl(retention_days)
+ try:
+ max_per_run: int | None = validate_retention_cap(
+ current_app.config.get(
+ "VERSION_HISTORY_PRUNE_MAX_TRANSACTIONS_PER_RUN", 1000
+ ),
+ "VERSION_HISTORY_PRUNE_MAX_TRANSACTIONS_PER_RUN",
+ )
+ except ValueError:
+ logger.warning("version_history_retention: invalid prune cap;
skipping")
+
stats_logger_manager.instance.incr(f"{_METRIC_PREFIX}.skipped_invalid_cap")
+ return {"skipped_invalid_cap": 1}
+ dry_run: bool =
current_app.config.get("VERSION_HISTORY_PRUNE_DRY_RUN", False)
+ if type(dry_run) is not bool:
+ raise ValueError("VERSION_HISTORY_PRUNE_DRY_RUN must be a bool")
Review Comment:
A non-bool here raises into the broad `except` below, so it logs a full
"task failed" traceback, bumps `.failed`, and returns `{"error": 1}`. That
looks like a broken prune to whoever is on call, while a bad cap just above
gets a clean `skipped_invalid_cap`.
Worth handling it the same way, with a warning, a `skipped_invalid_dry_run`
counter, and an early return?
##########
superset/tasks/version_history_retention.py:
##########
@@ -351,8 +352,46 @@ def _chunked(items: list[int], size: int) ->
Iterator[list[int]]:
_METRIC_PREFIX: str = "superset.versioning.retention"
+def _live_bearing_tables(tables: ShadowTables) -> list[sa.Table]:
+ """Return every shadow table that can anchor a live transaction."""
+ live_bearing_tables: list[sa.Table] = [*tables.parent, *tables.child]
+ if tables.m2m is not None:
+ live_bearing_tables.append(tables.m2m)
+ return live_bearing_tables
+
+
+def _count_prunable(cutoff: datetime, tables: ShadowTables) -> int:
+ """Count the full eligible transaction backlog without writing rows."""
+ total: int = 0
+ after_id: int = 0
+ live_bearing_tables: list[sa.Table] = _live_bearing_tables(tables)
+ conn: sa.engine.Connection
+ while True:
+ with db.engine.connect() as conn:
+ window: _PruneWindow = _resolve_prune_window(
+ conn, cutoff, live_bearing_tables, after_id, _MAX_PRUNE_BATCH
+ )
+ total += len(window.prunable)
+ if window.candidate_count < _MAX_PRUNE_BATCH:
+ return total
+ after_id = window.max_candidate_id
+
+
+def _probe_prunable(cutoff: datetime, tables: ShadowTables) -> tuple[int,
bool]:
+ """Count one candidate window and say whether it covers the backlog."""
+ conn: sa.engine.Connection
+ with db.engine.connect() as conn:
+ window: _PruneWindow = _resolve_prune_window(
+ conn, cutoff, _live_bearing_tables(tables), 0, _MAX_PRUNE_BATCH
Review Comment:
The probe always starts at id 0, but after a capped run everything below the
last window is either already pruned or a live anchor. On an instance with a
lot of charts and dashboards nobody has touched in 30+ days, the first 1000
aged candidates end up being all anchors, so this reports
`remaining_eligible=0` with `remaining_count_complete=False` on every run and
the gauge never moves.
Could `_add_prune_cap_stats` take the `after_id` the loop stopped at and
probe from there? The remainder would then land on the real backlog. A unit
test with a window of preserved ids ahead of the prunable ones would cover it.
--
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]
