Replace CEL-based NoSQL maintenance retention with commit count configuration#5135
Replace CEL-based NoSQL maintenance retention with commit count configuration#5135iting0321 wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR replaces CEL-expression-based retention rules for NoSQL metastore maintenance with a simpler commit-count configuration, and removes the now-unused CEL retention module/dependencies across the build and distribution artifacts.
Changes:
- Replace
*-retainCEL expressions with@Min(1)integer commit-count settings (defaulting to1) for catalog maintenance retention. - Remove the CEL retention module and its dependency wiring (Gradle projects, BOM, versions catalog), and tighten Checkstyle’s relocated-package ban accordingly.
- Add commit-count retention tests and update user documentation/migration guidance.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| site/content/in-dev/unreleased/configuration/config-sections/smallrye-polaris_persistence_nosql_maintenance_catalog.md | Updates the config reference docs to describe *-retain as int commit counts with default 1. |
| site/content/in-dev/unreleased/admin-tool.md | Adds an “important” upgrade note explaining the migration away from CEL expressions. |
| runtime/server/distribution/LICENSE | Removes the bundled dependency notice for Projectnessie CEL. |
| runtime/admin/distribution/LICENSE | Removes the bundled dependency notice for Projectnessie CEL. |
| persistence/nosql/persistence/metastore-maintenance/src/testFixtures/java/org/apache/polaris/persistence/nosql/metastore/maintenance/MutableCatalogsMaintenanceConfig.java | Updates test-fixture config wrapper to return int retention values. |
| persistence/nosql/persistence/metastore-maintenance/src/test/java/org/apache/polaris/persistence/nosql/metastore/maintenance/TestCommitRetention.java | Adds focused tests for commit-count retention behavior and invalid values. |
| persistence/nosql/persistence/metastore-maintenance/src/test/java/org/apache/polaris/persistence/nosql/metastore/maintenance/TestCatalogMaintenance.java | Updates existing maintenance tests to configure retention via commit counts. |
| persistence/nosql/persistence/metastore-maintenance/src/main/java/org/apache/polaris/persistence/nosql/metastore/maintenance/CatalogsMaintenanceConfig.java | Switches config mapping from optional CEL strings to validated int commit-count properties with defaults. |
| persistence/nosql/persistence/metastore-maintenance/src/main/java/org/apache/polaris/persistence/nosql/metastore/maintenance/CatalogRetainedIdentifier.java | Replaces CEL-based “continue” predicates with a commit-count predicate for identifying retained objects. |
| persistence/nosql/persistence/metastore-maintenance/build.gradle.kts | Drops the CEL maintenance module dependency. |
| persistence/nosql/persistence/maintenance/spi/src/main/java/org/apache/polaris/persistence/nosql/maintenance/spi/RetainedCollector.java | Removes references to the CEL predicate from SPI documentation/imports. |
| persistence/nosql/persistence/maintenance/spi/build.gradle.kts | Removes compileOnly dependency on the CEL maintenance module. |
| persistence/nosql/persistence/maintenance/retain-cel/src/test/java/org/apache/polaris/maintenance/cel/TestCelReferenceContinuePredicate.java | Deletes CEL predicate tests (module removal). |
| persistence/nosql/persistence/maintenance/retain-cel/src/main/java/org/apache/polaris/maintenance/cel/CelReferenceContinuePredicate.java | Deletes the CEL-based reference continue predicate implementation. |
| persistence/nosql/persistence/maintenance/retain-cel/src/jmh/java/org/apache/polaris/maintenance/cel/CelReferenceContinuePredicateBench.java | Deletes the CEL predicate benchmark (module removal). |
| persistence/nosql/persistence/maintenance/retain-cel/build.gradle.kts | Removes the Gradle module definition for the CEL retention project. |
| gradle/projects.main.properties | Removes the CEL module from the Gradle project map. |
| gradle/libs.versions.toml | Removes the CEL BOM entry from the versions catalog. |
| codestyle/checkstyle.xml | Removes the CEL-related exception and bans all *.relocated.* imports again. |
| bom/build.gradle.kts | Removes the CEL module from the published BOM dependencies. |
8f0ecf4 to
5030f3e
Compare
dimas-b
left a comment
There was a problem hiding this comment.
Thanks for picking up #3847, @iting0321 !
The PR LGTM with a couple of minor comments.
| The catalog-history `*-retain` settings now accept a positive integer number of latest commits | ||
| instead of a CEL expression. When upgrading, replace `*-retain=false` with `*-retain=1`. | ||
| CEL values such as `true` and expressions using `ageDays`, `ageHours`, or `ageMinutes` are no | ||
| longer supported. There is no unbounded or time-based equivalent; choose an explicit commit count |
There was a problem hiding this comment.
This looks like a reasonable compromise for now.
| var retainThree = CatalogRetainedIdentifier.<Object>retainLatest(3); | ||
| assertThat(retainThree.test(new Object())).isTrue(); | ||
| assertThat(retainThree.test(new Object())).isTrue(); | ||
| assertThat(retainThree.test(new Object())).isFalse(); |
There was a problem hiding this comment.
This is a bit misleading. I'd expect three test calls to return true 🤔
E.g. the old CEL expression commits <= 3 would be true on three calls, right?
There was a problem hiding this comment.
Thanks for your feedback!
The behavior for RetainedCollector.refRetain() retains the current commit before evaluating
the predicate:
var obj = iter.next();
retainedObjConsumer.accept(obj); // current commit is retained first
if (!continuePredicate.test(obj)) {
break;
}Therefore, true, true, false retains exactly three commits the predicate determines whether traversal should continue to the next commit.
I agree the name is misleading so I rename the method to continueWhileMoreCommitsRemain.
|
|
||
| @Override | ||
| public boolean test(O ignored) { | ||
| return --remaining > 0; |
There was a problem hiding this comment.
This can underflow (after many calls).
snazy
left a comment
There was a problem hiding this comment.
I don’t think this is ready to merge yet.
NoSQL pagination tokens contain a reference to historic state, subsequent requests need that exact object. If maintenance has already collected it, pagination would break in a subtle way, after an arbitrarily short time.
Could we define the expected token behavior across maintenance and add a test covering this interaction? Depending on the outcome, we may need to preserve a time-based retention dimension, though not necessarily CEL itself.
Please also verify the migration mapping for commits <= N: since the current commit is retained before evaluating the predicate, the old expression appears to retain N + 1, not N.
Requesting changes pending that investigation.
| Optional<String> principalsRetain(); | ||
| /** Number of latest principal commits to retain. */ | ||
| @WithDefault("" + DEFAULT_RETAIN_COMMITS) | ||
| @Min(1) |
There was a problem hiding this comment.
Is @Min actually checked here (maybe via Quarkus)?
There was a problem hiding this comment.
Yes, Quarkus should enforce validation annotations on its managed config objects (worth double-checking, though 😅 )
Hi @snazy, thanks for your feedback! For pagination, I defined the following behavior:
I added an integration test that creates a pagination token, supersedes its snapshot, runs maintenance, and verifies that:
You were also correct about the migration mapping. Because the current commit is retained before the predicate is evaluated:
I corrected this in both the changelog and migration documentation. |
3fa56d5 to
89f3a55
Compare
Summary
Fixes #3847 .
This PR replaces CEL-based retention rules in NoSQL metastore maintenance with simple positive integer configuration values.
Each
*-retainproperty now specifies the number of latest commits to retain. All properties default to1, preserving the current behavior of retaining only the latest commit.This PR also:
*-retain=falsewith*-retain=1.CEL values such as
trueand expressions usingageDays,ageHours, orageMinutesare no longer supported.Migration
This is a user-facing configuration change. Existing CEL values must be replaced with commit counts.
false1commits <= NN+1trueageDays,ageHours, orageMinutesFor example:
This PR does not introduce a
catalog-state-retain-daysproperty. The current behavior was deliberately changed by #3483 to retain only the latest catalog state, so adding a 30-day default would restore previously removed behavior rather than preserve the existing behavior.Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)