Core: Decompose BasePersistence into disjoint SPIs by huaxingao · Pull Request #4397 · apache/polaris

huaxingao · 2026-05-10T06:28:53Z

Refactor the persistence SPIs so BasePersistence, PolicyMappingPersistence, MetricsPersistence, and IntegrationPersistence are disjoint top-level interfaces instead of BasePersistence being an aggregator. Concrete backends (JDBC, NoSQL) multi-inherit the ones they support, and TransactionalPersistence aggregates them where atomic semantics are needed.

Changes

BasePersistence no longer extends PolicyMappingPersistence or MetricsPersistence.
TransactionalPersistence now extends all four SPIs directly.
JdbcBasePersistenceImpl and NonFunctionalBasePersistence implement all four SPIs.
MetaStoreManagerFactory: rename getOrCreateSession → getOrCreateBasePersistence; add getOrCreate{PolicyMapping,Metrics,Integration}Persistence.
PolarisCallContext: rename getMetaStore → getBasePersistence; add get{PolicyMapping,Metrics,Integration}Persistence.
Replace ((IntegrationPersistence) ms) blind casts in AtomicOperationMetaStoreManager with callCtx.getIntegrationPersistence().

Checklist

🛡️ Don't disclose security issues! (contact security@apache.org)
🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
🧪 Added/updated tests with good coverage, or manually tested (and explained how)
💡 Added comments for complex logic
🧾 Updated CHANGELOG.md (if needed)
📚 Updated documentation in site/content/in-dev/unreleased (if needed)

huaxingao · 2026-05-11T17:07:09Z

@dennishuo @dimas-b @flyingImer @singhpk234
Could you please take a look at this refactor PR? Thanks a lot!

dimas-b · 2026-05-12T22:43:16Z


  @Override
-  public BasePersistence getOrCreateSession(RealmContext realmContext) {
+  public BasePersistence getOrCreateBasePersistence(RealmContext realmContext) {


nit: this rename causes insignificant changes in the context of this PR, which complicates review. It would be nicer to do the renaming before or after this PR 😅

Happy to split if you'd prefer. I kept it in this PR because Dennis asked for the rename in his mailing-list reply.

If you do not mind, let's do the rename separately and before this PR - it is a non-controversial rename - easy to review and approve :) then this (more difficult) PR will have a smaller diff (after rebasing).

However... I personally already reviewed those rename changes 😅 so from my POV they are ok to keep in this PR.

dimas-b · 2026-05-12T22:49:28Z

+   * implements {@link PolicyMappingPersistence}.
+   */
+  default PolicyMappingPersistence getOrCreatePolicyMappingPersistence(RealmContext realmContext) {
+    return cast(getOrCreateBasePersistence(realmContext), PolicyMappingPersistence.class);


TBH, I do not really understand the purpose behind this design. We're still forcing implementation classes to support all the same interfaces that used to be extended by BasePersistence. IMHO, this make the overall design more awkward than before.

I believe the idea is/was to inject PolicyMappingPersistence into callers independently of BasePersistence. If they happen to be implemented by the same class, it should just work. However, we should not assume that (and use casts).

You comment makes sense. I have updated the PR accordingly. PTAL

dimas-b · 2026-05-12T22:51:23Z

Thanks for working on this, @huaxingao ! Still, I believe this PR needs more work. It is moving in the right direction, though, from my POV 👍

dimas-b · 2026-05-13T14:46:15Z

+  PolicyMappingPersistence getOrCreatePolicyMappingPersistence(RealmContext realmContext);
+
+  /** Returns the per-realm {@link MetricsPersistence}. */
+  MetricsPersistence getOrCreateMetricsPersistence(RealmContext realmContext);


MetricsPersistence is very different from "meta store" persistence... both functionally and in java SPI parameters. I believe it makes sense to use a separate factory for it.

That said, current changes are net positive, IMHO, and are moving in the right direction.

I think we can deal with the MetricsPersistence factory in a separate PR.

dimas-b · 2026-05-13T14:48:00Z


  @Override
-  public BasePersistence getOrCreateSession(RealmContext realmContext) {
+  public BasePersistence getOrCreateBasePersistence(RealmContext realmContext) {


However... I personally already reviewed those rename changes 😅 so from my POV they are ok to keep in this PR.

dimas-b · 2026-05-13T14:57:26Z

+
+  @Override
+  public MetricsPersistence getOrCreateMetricsPersistence(RealmContext realmContext) {
+    return newNoSqlMetaStore(realmContext);


I think, this should return a no-op new MetricsPersistence() {} for the sake of clarity - NoSQL persistence does not implement it anyway.

dimas-b

Thanks again for working on this, @huaxingao !

Sorry for pivoting, but I think we're making positive progress overall 😅

Let's concentrate on isolating MetricsPersistence and IdempotencyPersistence from BasePersistence for now. Old use cases may be too cumbersome and can remain "as is" as far as #4269 is concerned.

dimas-b · 2026-05-13T15:01:16Z

+abstract class NonFunctionalBasePersistence
+    implements BasePersistence,
+        IntegrationPersistence,
+        MetricsPersistence,


Please remove MetricsPersistence from this list - it's not really implemented.

dimas-b · 2026-05-13T15:05:48Z

        schemaVersion);
  }

+  private JdbcBasePersistenceImpl getOrCreateJdbcPersistence(RealmContext realmContext) {


there's no "get" logic in this method, it always "creates". Please consider renaming to createJdbcPersistence() for clarity.

dimas-b · 2026-05-13T15:15:21Z

-        ms.deleteAllEntityPolicyMappingRecords(callCtx, entity, mappingOnTarget, mappingOnPolicy);
+                : policyMappingPersistence.loadAllPoliciesOnTarget(
+                    callCtx, entity.getCatalogId(), entity.getId());
+        policyMappingPersistence.deleteAllEntityPolicyMappingRecords(


I think this goes against the grain of AtomicOperationMetaStoreManager javadoc, which says the class operates on one BasePersistence instance (logically).

IMHO, even the old code is not "atomic" with respect to ms.deleteAllEntityGrantRecords (line 213) and deleteAllEntityPolicyMappingRecords (line 236), but let's not aggravate this situation by introducing different objects. The old code at least invoked both methods on the same java object instance.

Sorry for missing this aspect when I initially suggested breaking BasePersistece down into all individual sub-interfaces. This code is not super easy to comprehend and keep in mind all the time 😅

With this code in mind, I think it makes sense to isolate only MetricsPersistence in this PR (and isolate idempotency persistence in #4269).

I think we need to keep this general difficulty (atomic operations) in mind for future BasePersistece refactorings, but for now I think it makes sense to keep "old" functionality intact and only isolate "new" functionality into purpose-based interfaces (MetricsPersistence, IdempotencyPersistence, etc.)

dimas-b

Thanks for bearing with me, @huaxingao 😅

huaxingao · 2026-05-13T22:16:04Z

Thanks so much for the careful review, @dimas-b! Appreciate the guidance on this PR!

flyingImer

Nice first step toward making persistence SPIs independently implementable. Two small suggestions below to smooth the path for out-of-tree integrators.

flyingImer · 2026-05-13T22:22:33Z

+   * BasePersistence} so backends that do not implement metrics persistence can simply return a
+   * no-op instance.
+   */
+  MetricsPersistence getOrCreateMetricsPersistence(RealmContext realmContext);


Consider giving this a default implementation:

default MetricsPersistence getOrCreateMetricsPersistence(RealmContext realmContext) { return new MetricsPersistence() {}; }

MetricsPersistence methods are all no-op by default, so the factory should be too. Backends that don't implement metrics won't need to touch their factory class. They get silent no-op for free, and the source-breaking surface shrinks.

Thanks for the suggestion! I'd prefer to keep this abstract. @dimas-b 's earlier comment on the NoSQL override asked for an explicit new MetricsPersistence() {} "for the sake of clarity", my read is we should avoid a factory-level default.

+1 to avoid factory-level defaults.

Also, I suggested making a separate factory for MetricsPersistence as a follow-up. If we go this way, implementation will always be complete (no need for default methods).

flyingImer · 2026-05-13T22:23:08Z

  }

+  @Override
+  public MetricsPersistence getOrCreateMetricsPersistence(RealmContext realmContext) {


Nit: stateless, so a private static final singleton avoids per-call allocation.

Core: Decompose BasePersistence into disjoint SPIs

8402073

github-project-automation Bot added this to Basic Kanban Board May 10, 2026

github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board May 10, 2026

huaxingao mentioned this pull request May 12, 2026

Add handler-level Idempotency-Key support for createTableDirect #4269

Open

6 tasks

dimas-b reviewed May 12, 2026

View reviewed changes

address comment

e514ca5

dimas-b reviewed May 13, 2026

View reviewed changes

Only isolate MetricsPersistence from BasePersistence

b745cd6

dimas-b previously approved these changes May 13, 2026

View reviewed changes

github-project-automation Bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board May 13, 2026

flyingImer reviewed May 13, 2026

View reviewed changes

Reuse a static singleton for NoSQL no-op MetricsPersistence

93438f2

huaxingao dismissed dimas-b’s stale review via 93438f2 May 13, 2026 22:51

dimas-b approved these changes May 13, 2026

View reviewed changes

Conversation

huaxingao commented May 10, 2026

Checklist

Uh oh!

huaxingao commented May 11, 2026

Uh oh!

dimas-b May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b commented May 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

huaxingao commented May 13, 2026

Uh oh!

flyingImer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dimas-b May 12, 2026 •

edited

Loading

dimas-b May 13, 2026 •

edited

Loading