Request-scoped PolarisMetaStoreManager #2555

XN137 · 2025-09-12T16:37:15Z

see ML discussion:
https://lists.apache.org/thread/ot19px6t0w808pp994rrc8kk7186dfxm

the PolarisMetaStoreManager interface methods all take a PolarisCallContext parameter
because they need access to PolarisCallContext.getMetaStore aka the persistence session.
also occasionally the RealmContext and RealmConfig is needed.

this means PolarisMetaStoreManager is only usable within a request-scope.
MetaStoreManagerFactory.getOrCreateMetaStoreManager impls suggests that there needs to
be one shared PolarisMetaStoreManager instance per realm.
however if we look at the PolarisMetaStoreManager impls (AtomicOperationMetaStoreManager
and TransactionalMetaStoreManagerImpl) we can see that they dont have any state themselves, the fields only reference application-scoped instances.

Those 2 observations combined mean that we can make the PolarisMetaStoreManager request-scoped and then the PolarisCallContext parameter can be removed from all the methods without losing any functionality/flexibility.
note that if custom implementations would need any state sharing for a realm, they are still free to do so in their custom MetaStoreManagerFactory implementations, as they can
initialize the custom request-scoped PolarisMetaStoreManager instance with whatever (shared) data they need.

when PolarisMetaStoreManager is request-scoped the getOrCreateSession method also becomes a private implementation detail of the MetaStoreManagerFactory.

dimas-b

partial review

dimas-b · 2025-10-08T21:55:11Z

...rc/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java

+  public PolarisMetaStoreManager createMetaStoreManager(
+      RealmContext realmContext, @Nullable RealmConfig realmConfig) {
+    if (realmConfig == null) {
+      realmConfig = new RealmConfigImpl(configurationStore, realmContext);


Do we need the realmConfig parameter at all?

as we can build it ourselves its not strictly needed no, but many call sites already have it and can pass it in.
i am fine to remove the parameter if we think thats better overall.

dimas-b · 2025-10-08T23:17:33Z

polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/InMemoryEntityCache.java

 public class InMemoryEntityCache implements EntityCache {

  private final PolarisDiagnostics diagnostics;
-  private final PolarisMetaStoreManager polarisMetaStoreManager;


I suppose InMemoryEntityCache must use a PolarisMetaStoreManager from the same realm. If PolarisMetaStoreManager is a method parameter it become not so obvious. WDYT about keeping this field?

afaict we cant keep this field as PolarisMetaStoreManager now represents a (per-request) persistence session where-as InMemoryEntityCache is supposed to be a shared cache within a realm.

the question is with what persistence session do we want to populate the cache?
a shared one or the request-scoped one given by the callers as "use this for loading in case cache entry is missing" ?

If the cache answers from memory, the per-request persistence session is irrelevant. So the caller of the lookup method(s) does not know whether the current persistence session was / will be used. Some values returned from the cache might have been loaded via a different session.

The "reload if necessary" checks are based on local entity data and do not appear to take the "session" into account.

With that in mind, I think it would be preferable to resolve this ambiguity and always load from a separate session (one per InMemoryEntityCache instance or created on demand).

@dennishuo : WDYT?

XN137 · 2025-10-09T09:41:27Z

rebased after trivial a conflict and also to adjust to AccessConfigProvider (though we need it to become request-scoped as well soon)

dimas-b

The general direction of this PR LGTM (but I have not reviewed all changes yet). Dealing with PolarisMetaStoreManager instances as per-request objects makes sense to me (irrespective of the runtime framework in use). The metastore factory controls the creation of PolarisMetaStoreManager objects and associating them with request-specific data.

@dennishuo @collado-mike WDYT?

dimas-b · 2025-10-17T14:53:20Z

runtime/service/src/main/java/org/apache/polaris/service/auth/internal/broker/JWTBroker.java


-  JWTBroker(PolarisMetaStoreManager metaStoreManager, int maxTokenGenerationInSeconds) {
-    this.metaStoreManager = metaStoreManager;
+  JWTBroker(int maxTokenGenerationInSeconds) {


TokenBroker is already request-scoped, why not rely on the constructor to have the correct metaStoreManager for the current request?

dimas-b · 2025-10-17T14:56:19Z

...ervice/src/main/java/org/apache/polaris/service/auth/internal/broker/TokenBrokerFactory.java

-public interface TokenBrokerFactory extends Function<RealmContext, TokenBroker> {}
+public interface TokenBrokerFactory {
+
+  TokenBroker newTokenBroker(RealmContext realmContext);


suggestion: move this into a separate PR maybe? I think it's a valuable refactoring and independent of the request scope discussions.

dimas-b · 2025-10-17T15:28:01Z

polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/InMemoryEntityCache.java

 public class InMemoryEntityCache implements EntityCache {

  private final PolarisDiagnostics diagnostics;
-  private final PolarisMetaStoreManager polarisMetaStoreManager;


If the cache answers from memory, the per-request persistence session is irrelevant. So the caller of the lookup method(s) does not know whether the current persistence session was / will be used. Some values returned from the cache might have been loaded via a different session.

The "reload if necessary" checks are based on local entity data and do not appear to take the "session" into account.

With that in mind, I think it would be preferable to resolve this ambiguity and always load from a separate session (one per InMemoryEntityCache instance or created on demand).

@dennishuo : WDYT?

the `PolarisMetaStoreManager` interface methods all take a `PolarisCallContext` parameter because they need access to `PolarisCallContext.getMetaStore` aka the persistence session. also occasionally the `RealmContext` and `RealmConfig` is needed. this means `PolarisMetaStoreManager` is only usable within a request-scope. `MetaStoreManagerFactory.getOrCreateMetaStoreManager` impls suggests that there needs to be one shared `PolarisMetaStoreManager` instance per realm. however if we look at the `PolarisMetaStoreManager` impls (`AtomicOperationMetaStoreManager` and `TransactionalMetaStoreManagerImpl`) we can see that they dont have any state themselves, the fields only reference application-scoped collaborators. those 2 observations combined mean that in order to reduce the dependency on `PolarisCallContext` the `PolarisMetaStoreManager` has to become request-scoped. note that if custom implementations would need any state sharing for a realm, they are still free to do so in their custom `MetaStoreManagerFactory` implementations, as they can initialize the request-scoped `PolarisMetaStoreManager` with whatever data they need. when `PolarisMetaStoreManager` is request-scoped the `getOrCreateSession` method also becomes a private implementation detail of the `MetaStoreManagerFactory`. the `EntityCache` / `InMemoryEntityCache` however can no longer operate on a single `PolarisMetaStoreManager` and the one from the request needs to be passed as a parameter in all the methods now.

this avoids `TokenBrokerFactory` impls having to call `MetaStoreManagerFactory.createMetaStoreManager`

snazy · 2025-11-04T18:14:26Z

Looks like this PR needs a rebase

github-project-automation bot added this to Basic Kanban Board Sep 12, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Sep 12, 2025

XN137 force-pushed the request-scoped-PolarisMetaStoreManager branch 4 times, most recently from 9738d71 to c014ca0 Compare September 15, 2025 07:54

XN137 mentioned this pull request Sep 24, 2025

Remove PolarisCallContext.getMetaStore #2229

Closed

XN137 force-pushed the request-scoped-PolarisMetaStoreManager branch 8 times, most recently from c1b2564 to 4511b5e Compare October 1, 2025 06:19

XN137 force-pushed the request-scoped-PolarisMetaStoreManager branch 3 times, most recently from ebc2443 to e9f1079 Compare October 6, 2025 08:31

XN137 marked this pull request as ready for review October 6, 2025 15:40

XN137 force-pushed the request-scoped-PolarisMetaStoreManager branch from e9f1079 to 88db62e Compare October 7, 2025 13:30

dimas-b mentioned this pull request Oct 7, 2025

Extract IcebergCatalog.getAccessConfig to a separate class AccessConfigProvider #2736

Merged

XN137 force-pushed the request-scoped-PolarisMetaStoreManager branch from 88db62e to d1dda2e Compare October 8, 2025 08:36

dimas-b requested review from HonahX and dennishuo October 8, 2025 21:10

dimas-b reviewed Oct 8, 2025

View reviewed changes

XN137 force-pushed the request-scoped-PolarisMetaStoreManager branch from d1dda2e to 37696b7 Compare October 9, 2025 09:40

XN137 force-pushed the request-scoped-PolarisMetaStoreManager branch 3 times, most recently from faf5210 to 0eb0f2b Compare October 14, 2025 12:38

XN137 force-pushed the request-scoped-PolarisMetaStoreManager branch from 0eb0f2b to 1a1ddb5 Compare October 17, 2025 08:45

dimas-b reviewed Oct 17, 2025

View reviewed changes

dimas-b requested a review from collado-mike October 17, 2025 15:36

XN137 force-pushed the request-scoped-PolarisMetaStoreManager branch from 1a1ddb5 to 3d96588 Compare October 18, 2025 06:32

XN137 added 2 commits October 23, 2025 17:33

inject PolarisMetaStoreManager into DefaultOAuth2ApiService

345212f

this avoids `TokenBrokerFactory` impls having to call `MetaStoreManagerFactory.createMetaStoreManager`

XN137 force-pushed the request-scoped-PolarisMetaStoreManager branch from 3d96588 to 345212f Compare October 23, 2025 15:34

Request-scoped PolarisMetaStoreManager #2555

Are you sure you want to change the base?

Request-scoped PolarisMetaStoreManager #2555

Uh oh!

Conversation

XN137 commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XN137 commented Oct 9, 2025

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!

snazy commented Nov 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

XN137 commented Sep 12, 2025 •

edited

Loading