feat(versioning): version-history retention cleanup job - #41075
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
8e13952 to
e7848e7
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #41075 +/- ##
========================================
Coverage 65.25% 65.26%
========================================
Files 2795 2796 +1
Lines 157639 157815 +176
Branches 36052 36078 +26
========================================
+ Hits 102869 102998 +129
- Misses 52793 52827 +34
- Partials 1977 1990 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ac1e444 to
2979f14
Compare
daf5db8 to
145356f
Compare
145356f to
997a89f
Compare
Apply code-review fixes to the version-history retention job (apache#41075): - Drop the version_transaction.issued_at index migration. The prune's candidate query (ORDER BY id LIMIT) is driven by the primary key on every supported engine (clustered on InnoDB, rowid on SQLite, PK index on Postgres) and would never choose a standalone issued_at index, which would only force a filesort before the LIMIT. ids are monotonic with issued_at, so the PK scan already reads prunable rows efficiently. Removing it also drops a write-blocking non-concurrent CREATE INDEX at deploy. - Match the retention beat entry by each entry's `task` value, not the schedule key, so a task registered under a different key no longer triggers a spurious startup warning. - Replace deprecated datetime.utcnow() with a tz-aware clock reduced to naive UTC (preserving the naive-vs-naive comparison against issued_at). - Fix docs referencing CELERYBEAT_SCHEDULE -> CeleryConfig.beat_schedule (task docstring, initializer comment, UPDATING.md). - Clarify that the OperationalError retry catch is intentionally broad (serialization failures plus transient DB errors, all safe to retry). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Apply code-review fixes to the version-history retention job (apache#41075): - Drop the version_transaction.issued_at index migration. The prune's candidate query (ORDER BY id LIMIT) is driven by the primary key on every supported engine (clustered on InnoDB, rowid on SQLite, PK index on Postgres) and would never choose a standalone issued_at index, which would only force a filesort before the LIMIT. ids are monotonic with issued_at, so the PK scan already reads prunable rows efficiently. Removing it also drops a write-blocking non-concurrent CREATE INDEX at deploy. - Match the retention beat entry by each entry's `task` value, not the schedule key, so a task registered under a different key no longer triggers a spurious startup warning. - Replace deprecated datetime.utcnow() with a tz-aware clock reduced to naive UTC (preserving the naive-vs-naive comparison against issued_at). - Fix docs referencing CELERYBEAT_SCHEDULE -> CeleryConfig.beat_schedule (task docstring, initializer comment, UPDATING.md). - Clarify that the OperationalError retry catch is intentionally broad (serialization failures plus transient DB errors, all safe to retry). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
922fe59 to
4acf1d9
Compare
There was a problem hiding this comment.
Code Review Agent Run #072250
Actionable Suggestions - 1
-
superset/migrations/versions/2026-06-15_16-30_d3b9a1f6c204_version_transaction_issued_at_index.py - 1
- Missing migration unit test · Line 1-48
Additional Suggestions - 1
-
tests/unit_tests/config_test.py - 1
-
Log capture level not configured · Line 50-60The `caplog` fixture captures logs at WARNING level by default, but the logger for `superset.config` may not have its level propagated in unit test contexts. Other tests in this repo use `caplog.at_level(logging.WARNING)` explicitly (see `tests/unit_tests/mcp_service/test_auth_safeguards.py:205`). Without this, the log assertion may pass inconsistently depending on pytest configuration.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
tests/integration_tests/versioning/retention_prune_tests.py - 2
- Missing session cleanup · Line 356-356
- CWE-477: Deprecated datetime.utcnow() · Line 163-163
Review Details
-
Files reviewed - 12 · Commit Range:
3b249a2..4acf1d9- superset/config.py
- superset/initialization/__init__.py
- superset/mcp_service/sql_lab/tool/execute_sql.py
- superset/migrations/versions/2026-06-15_16-30_d3b9a1f6c204_version_transaction_issued_at_index.py
- superset/tasks/celery_app.py
- superset/tasks/version_history_retention.py
- superset/versioning/queries.py
- tests/integration_tests/versioning/retention_prune_tests.py
- tests/unit_tests/config_test.py
- tests/unit_tests/initialization_test.py
- tests/unit_tests/semantic_layers/mapper_test.py
- tests/unit_tests/tasks/test_version_history_retention.py
-
Files skipped - 1
- UPDATING.md - Reason: Filter setting
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
4acf1d9 to
3d75452
Compare
b415827 to
db87a6b
Compare
Code Review Agent Run #30a403Actionable Suggestions - 0Additional Suggestions - 2
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
rusackas
left a comment
There was a problem hiding this comment.
Good timing, #41176 landed, so this isn't stacked anymore, diff looks simpler. One thing I'm not following though why are execute_sql.py and the semantic_layers/mapper_test.py tweaks are in here, or if either related to the retention job. Maybe bycatch, or just part of the bigger plan?
rusackas
left a comment
There was a problem hiding this comment.
Good timing, #41176 landed, so this isn't stacked anymore, diff looks simpler. One thing I'm not following though why are execute_sql.py and the semantic_layers/mapper_test.py tweaks are in here, or if either related to the retention job. Maybe bycatch, or just part of the bigger plan?
A Celery beat task that prunes version_transaction / version_changes / shadow rows beyond the configured retention window, so the version tables don't grow unbounded once capture is enabled. Preserves live child/M2M rows, batches the deletes with bind/retry hardening, and adds an index on version_transaction.issued_at for the prune scan. Stacked on the versioning base infrastructure (apache#41176). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Apply review fixes to the version-history retention job: retain and document the issued_at index based on planner testing, add migration contract tests, match Celery beat entries by task value, use timezone-aware UTC, correct configuration documentation, and clarify retry handling.
db87a6b to
2af1df9
Compare
The SQLAlchemy 2.0 enforcement added on master turns the implicit- autocommit RemovedIn20Warning into an error; run the migration DDL under engine.begin() instead of a bare connection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review Agent Run #4b8916
Actionable Suggestions - 1
-
tests/integration_tests/versioning/retention_prune_tests.py - 1
- Deprecated datetime.utcnow() in test · Line 432-432
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
tests/integration_tests/versioning/retention_prune_tests.py - 5
- Deprecated datetime.utcnow() in test · Line 163-163
- Deprecated datetime.utcnow() in test · Line 260-260
- Deprecated datetime.utcnow() call · Line 163-163
- Deprecated datetime.utcnow() call · Line 260-260
- Deprecated datetime.utcnow() call · Line 432-432
Review Details
-
Files reviewed - 13 · Commit Range:
2038b41..77b4c56- superset/config.py
- superset/initialization/__init__.py
- superset/mcp_service/sql_lab/tool/execute_sql.py
- superset/migrations/versions/2026-07-27_10-00_d3b9a1f6c204_version_transaction_issued_at_index.py
- superset/tasks/celery_app.py
- superset/tasks/version_history_retention.py
- superset/versioning/queries.py
- tests/integration_tests/versioning/retention_prune_tests.py
- tests/unit_tests/config_test.py
- tests/unit_tests/initialization_test.py
- tests/unit_tests/migrations/test_version_transaction_issued_at_index.py
- tests/unit_tests/semantic_layers/mapper_test.py
- tests/unit_tests/tasks/test_version_history_retention.py
-
Files skipped - 1
- UPDATING.md - Reason: Filter setting
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
Replace the three deprecated datetime.utcnow() call sites in the prune tests with the naive-UTC form the fourth site already uses, and soften the version-number loop docstring: transactions still anchoring a live row survive past the retention window, bounded by entity size rather than edit volume. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Code Review Agent Run #76f279Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
SUMMARY
Adds a time-based version-history retention cleanup job for the entity-versioning feature. As entities (dashboards, charts, datasets and their columns/metrics) are edited, the versioning subsystem accumulates shadow rows in
*_versiontables plusversion_transaction/version_changesrecords. Without pruning, those tables grow unbounded. This PR adds a daily Celery beat task that deletes version history older than a configurable window while always preserving each entity's current (live) state.API: none (background Celery beat task). This PR adds no REST/HTTP surface — only a Celery task, a config key, an index migration, and a startup sanity check.
Depends on the entity-versioning base infrastructure (#41176), which has merged — this PR's diff is standalone.
What this adds
superset/tasks/version_history_retention.py— theversion_history.prune_old_versionsCelery task. For everyversion_transactionwhoseissued_atis older than the retention window it deletes the owned parent + child + M2M (dashboard_slices_version) shadow rows and theversion_transactionrow itself;version_changesrows are removed byON DELETE CASCADE.end_transaction_id IS NULL) — parent, child, or the M2M association — the whole transaction and all of its shadow/change rows are kept, regardless of age. This keeps a still-live child (e.g. an unedited column anchored at an older transaction than its parent's current row) from being silently stripped. Baseline rows (operation_type = 0) and closed historical rows age out like everything else.SUPERSET_VERSION_HISTORY_RETENTION_DAYSconfig (default30, read from the env var of the same name;0or negative disables the prune and short-circuits to a no-op). Read from the global config; a per-workspace override is a planned follow-up and is not in this PR.CELERYBEAT_SCHEDULEentryversion_history.prune_old_versions, daily at 03:00.d3b9a1f6c204— btree index onversion_transaction.issued_atso the candidate scan is a range scan, not a full-table scan. Idempotent (create_indexskips if present)._warn_if_retention_beat_missingcheck — logs a WARN when an operator's customCELERY_CONFIGsilently drops the beat entry (capture keeps writing rows but the prune never runs, so disk grows unbounded).Safety / correctness properties
SQLITE_MAX_VARIABLE_NUMBERfloor.superset.versioning.retention.*(pruned_transactionsgauge per run;.skippedwhen disabled / no versioned classes;.retriedon serialization conflict;.failedon terminal failure). Runs entirely off the save path on its own DB connection.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only, no UI.
TESTING INSTRUCTIONS
QA NOTES
Set
SUPERSET_VERSION_HISTORY_RETENTION_DAYSand back-dateversion_transaction.issued_atto simulate aged history:issued_at < now - N days).end_transaction_id IS NULL); the historical chain (including the synthetic baseline) is gone for entities not edited within the window.dashboard_slices_versionassociation stays live at an older transaction than the parent's current live row; confirm the prune does not delete that still-live child/association even though its anchoring transaction predates the cutoff.0disables: set to0(and a negative number); confirm no-op +superset.versioning.retention.skipped, no deletions.pruned_transactions == 0).version_transactionrows take theirversion_changesrows with them; no orphaned shadow rows remain.CELERY_CONFIGwithout theversion_history.prune_old_versionsentry; confirm the startup WARN fires.d3b9a1f6c204; confirmix_version_transaction_issued_atis created/dropped and re-applying upgrade is safe.ADDITIONAL INFORMATION
version_transaction.issued_atSUPERSET_VERSION_HISTORY_RETENTION_DAYSconfig key — no HTTP API)Builds on #41176 (base infrastructure, merged).