Drain JDBC realm time-series data via a batched purge command#5089
Drain JDBC realm time-series data via a batched purge command#5089ayushtkn wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Extends JDBC metastore cleanup so JdbcBasePersistenceImpl.deleteAll() purges additional realm-scoped tables introduced in newer schema versions (events in v3+, scan/commit metrics report tables in v4+), preventing stale data from remaining after a realm-scoped delete.
Changes:
- Update
JdbcBasePersistenceImpl.deleteAll()to delete fromevents,scan_metrics_report, andcommit_metrics_reportwhen the schema version supports them. - Add a new integration-style test that populates all realm-scoped tables in schema v5 and verifies
deleteAll()purges only the target realm.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java | Extends deleteAll() to delete newer realm-scoped tables, gated by schema version. |
| persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImplTest.java | Adds coverage verifying realm-scoped purge behavior across all relevant tables for schema v5. |
| if (schemaVersion >= 3) { | ||
| datasourceOperations.execute( | ||
| connection, | ||
| QueryGenerator.generateDeleteQuery( | ||
| ModelEvent.ALL_COLUMNS, ModelEvent.TABLE_NAME, params)); | ||
| } | ||
| if (schemaVersion >= 4) { | ||
| datasourceOperations.execute( | ||
| connection, | ||
| QueryGenerator.generateDeleteQuery( | ||
| ModelScanMetricsReport.ALL_COLUMNS, | ||
| ModelScanMetricsReport.TABLE_NAME, | ||
| params)); | ||
| datasourceOperations.execute( | ||
| connection, | ||
| QueryGenerator.generateDeleteQuery( | ||
| ModelCommitMetricsReport.ALL_COLUMNS, | ||
| ModelCommitMetricsReport.TABLE_NAME, | ||
| params)); | ||
| } |
There was a problem hiding this comment.
make sense. added tests for all versions
| connection, | ||
| QueryGenerator.generateDeleteQuery( | ||
| ModelScanMetricsReport.ALL_COLUMNS, | ||
| ModelScanMetricsReport.TABLE_NAME, |
There was a problem hiding this comment.
The javadoc on deleteAll mentions only "entity and grant record metadata".
deleteAll() is specified by BasePersistence, so I do not think it applies to events and metrics, which are under MetricsPersistence. It's currently detached from BasePersistence in terms of interfaces and its implementation is moving to extensions in #5068 🤔
I believe purging metric should be done under a separate method call and probably after merging #5068.
IMHO, conceptually, events should also not be bound to BasePersistence either. I'd like to isolate them link metrics and then add dedicated maintenance tasks for purging (see my other comment).
WDYT?
| datasourceOperations.execute( | ||
| connection, | ||
| QueryGenerator.generateDeleteQuery( | ||
| ModelEvent.ALL_COLUMNS, ModelEvent.TABLE_NAME, params)); |
There was a problem hiding this comment.
Events a time-series type of data. There may be a lot more of them in the database than Meta Store data. I'm not sure issuing a broad DELETE for them in one Tx is a robust approach.
I tend to think it would be preferable to handle their purge asynchronously and in a retriable / staged manner similar to NoSQL maintenance: https://github.com/apache/polaris/blob/apache-polaris-1.6.0/persistence/nosql/persistence/maintenance/api/src/main/java/org/apache/polaris/persistence/nosql/maintenance/api/package-info.java
|
Thanks @dimas-b — agreed. We shouldn’t extend I’ll revisit after #5068 and the linked PRs land, once metrics are decoupled from Thanks — I’ll park this for now and circle back. |
ayushtkn
changed the title
deleteAll(aBasePersistenceoperation) previously left a realm's time-series data —events,scan_metrics_report,commit_metrics_report— behind. Rather than fix that with a broad inline delete, this makes the time-series purge asynchronous, queued, and batched, matching somewhat the behavior of the NoSQL maintenance command.deleteAllno longer touches time-series tables. It atomically enqueues the realm into a newrealm_purgemarker table, as the last statement in its own transaction — so a purged realm is never left with orphaned time-series data, and the marker never commits without the metastore delete.purge-drain, processes the queue: it claims each realm under a lease (one active drainer per realm across nodes, via a conditionalUPDATE),deletesits rows in bounded batches, then clears the marker.A realm whose drain fails or is interrupted keeps its marker and is retried once its lease expires. It's safe to run repeatedly/concurrently and is meant to be scheduled periodically (e.g. once a day), like nosql maintenance-run.
Per-table purgers are discovered via
CDI (Instance<RealmDataPurger>), so adding a table means adding a producer, not editing the drainer.Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)