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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
