You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "XN137 (via GitHub)" <gi...@apache.org> on 2025/09/12 16:37:17 UTC
[PR] Request-scoped PolarisMetaStoreManager [polaris]
XN137 opened a new pull request, #2555:
URL: https://github.com/apache/polaris/pull/2555
WIP
note that since entitycache is shared within a realm and it can no longer use a single metastore manager
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Request-scoped PolarisMetaStoreManager [polaris]
Posted by "XN137 (via GitHub)" <gi...@apache.org>.
XN137 commented on PR #2555:
URL: https://github.com/apache/polaris/pull/2555#issuecomment-3385028582
rebased after trivial a conflict and also to adjust to `AccessConfigProvider` (though we need it to become request-scoped as well soon)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Request-scoped PolarisMetaStoreManager [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2555:
URL: https://github.com/apache/polaris/pull/2555#discussion_r2440302593
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/internal/broker/TokenBrokerFactory.java:
##########
@@ -18,11 +18,13 @@
*/
package org.apache.polaris.service.auth.internal.broker;
-import java.util.function.Function;
import org.apache.polaris.core.context.RealmContext;
/**
* Factory that creates a {@link TokenBroker} for generating and parsing. The {@link TokenBroker} is
* created based on the realm context.
*/
-public interface TokenBrokerFactory extends Function<RealmContext, TokenBroker> {}
+public interface TokenBrokerFactory {
+
+ TokenBroker newTokenBroker(RealmContext realmContext);
Review Comment:
suggestion: move this into a separate PR maybe? I think it's a valuable refactoring and independent of the request scope discussions.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/InMemoryEntityCache.java:
##########
@@ -43,19 +42,12 @@
public class InMemoryEntityCache implements EntityCache {
private final PolarisDiagnostics diagnostics;
- private final PolarisMetaStoreManager polarisMetaStoreManager;
Review Comment:
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?
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/internal/broker/JWTBroker.java:
##########
@@ -50,11 +49,9 @@ public abstract class JWTBroker implements TokenBroker {
private static final String CLAIM_KEY_PRINCIPAL_ID = "principalId";
private static final String CLAIM_KEY_SCOPE = "scope";
- private final PolarisMetaStoreManager metaStoreManager;
private final int maxTokenGenerationInSeconds;
- JWTBroker(PolarisMetaStoreManager metaStoreManager, int maxTokenGenerationInSeconds) {
- this.metaStoreManager = metaStoreManager;
+ JWTBroker(int maxTokenGenerationInSeconds) {
Review Comment:
`TokenBroker` is already request-scoped, why not rely on the constructor to have the correct `metaStoreManager` for the current request?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Request-scoped PolarisMetaStoreManager [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2555:
URL: https://github.com/apache/polaris/pull/2555#discussion_r2415219019
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/InMemoryEntityCache.java:
##########
@@ -43,19 +42,12 @@
public class InMemoryEntityCache implements EntityCache {
private final PolarisDiagnostics diagnostics;
- private final PolarisMetaStoreManager polarisMetaStoreManager;
Review Comment:
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?
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -196,52 +188,46 @@ public Map<String, BaseResult> purgeRealms(Iterable<String> realms) {
for (String realm : realms) {
RealmContext realmContext = () -> realm;
- PolarisMetaStoreManager metaStoreManager = getOrCreateMetaStoreManager(realmContext);
- BasePersistence session = getOrCreateSession(realmContext);
+ PolarisMetaStoreManager metaStoreManager = createMetaStoreManager(realmContext, null);
- PolarisCallContext callContext = new PolarisCallContext(realmContext, session);
- BaseResult result = metaStoreManager.purge(callContext);
+ BaseResult result = metaStoreManager.purge();
results.put(realm, result);
sessionSupplierMap.remove(realm);
- metaStoreManagerMap.remove(realm);
}
return Map.copyOf(results);
}
@Override
- public synchronized PolarisMetaStoreManager getOrCreateMetaStoreManager(
- RealmContext realmContext) {
- if (!metaStoreManagerMap.containsKey(realmContext.getRealmIdentifier())) {
- DatasourceOperations datasourceOperations = getDatasourceOperations();
- initializeForRealm(datasourceOperations, realmContext, null);
- checkPolarisServiceBootstrappedForRealm(realmContext);
+ public PolarisMetaStoreManager createMetaStoreManager(
+ RealmContext realmContext, @Nullable RealmConfig realmConfig) {
+ if (realmConfig == null) {
+ realmConfig = new RealmConfigImpl(configurationStore, realmContext);
Review Comment:
Do we need the `realmConfig` parameter at all?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Request-scoped PolarisMetaStoreManager [polaris]
Posted by "XN137 (via GitHub)" <gi...@apache.org>.
XN137 commented on code in PR #2555:
URL: https://github.com/apache/polaris/pull/2555#discussion_r2416139337
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -196,52 +188,46 @@ public Map<String, BaseResult> purgeRealms(Iterable<String> realms) {
for (String realm : realms) {
RealmContext realmContext = () -> realm;
- PolarisMetaStoreManager metaStoreManager = getOrCreateMetaStoreManager(realmContext);
- BasePersistence session = getOrCreateSession(realmContext);
+ PolarisMetaStoreManager metaStoreManager = createMetaStoreManager(realmContext, null);
- PolarisCallContext callContext = new PolarisCallContext(realmContext, session);
- BaseResult result = metaStoreManager.purge(callContext);
+ BaseResult result = metaStoreManager.purge();
results.put(realm, result);
sessionSupplierMap.remove(realm);
- metaStoreManagerMap.remove(realm);
}
return Map.copyOf(results);
}
@Override
- public synchronized PolarisMetaStoreManager getOrCreateMetaStoreManager(
- RealmContext realmContext) {
- if (!metaStoreManagerMap.containsKey(realmContext.getRealmIdentifier())) {
- DatasourceOperations datasourceOperations = getDatasourceOperations();
- initializeForRealm(datasourceOperations, realmContext, null);
- checkPolarisServiceBootstrappedForRealm(realmContext);
+ public PolarisMetaStoreManager createMetaStoreManager(
+ RealmContext realmContext, @Nullable RealmConfig realmConfig) {
+ if (realmConfig == null) {
+ realmConfig = new RealmConfigImpl(configurationStore, realmContext);
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Request-scoped PolarisMetaStoreManager [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #2555:
URL: https://github.com/apache/polaris/pull/2555#issuecomment-3487436621
Looks like this PR needs a rebase
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Request-scoped PolarisMetaStoreManager [polaris]
Posted by "XN137 (via GitHub)" <gi...@apache.org>.
XN137 commented on code in PR #2555:
URL: https://github.com/apache/polaris/pull/2555#discussion_r2416152818
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/InMemoryEntityCache.java:
##########
@@ -43,19 +42,12 @@
public class InMemoryEntityCache implements EntityCache {
private final PolarisDiagnostics diagnostics;
- private final PolarisMetaStoreManager polarisMetaStoreManager;
Review Comment:
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" ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org