adutra commented on code in PR #493:
URL: https://github.com/apache/polaris/pull/493#discussion_r1869219908
##########
polaris-service/src/main/java/org/apache/polaris/service/auth/DefaultPolarisAuthenticator.java:
##########
@@ -36,16 +36,7 @@ public Optional<AuthenticatedPolarisPrincipal>
authenticate(String credentials)
return getPrincipal(decodedToken);
}
- @Override
- public void setMetaStoreManagerFactory(MetaStoreManagerFactory
metaStoreManagerFactory) {
- super.setMetaStoreManagerFactory(metaStoreManagerFactory);
- if (tokenBrokerFactory instanceof HasMetaStoreManagerFactory) {
- ((HasMetaStoreManagerFactory) tokenBrokerFactory)
- .setMetaStoreManagerFactory(metaStoreManagerFactory);
- }
- }
-
- @JsonProperty("tokenBroker")
+ @Inject
Review Comment:
Please turn into constructor injection.
##########
polaris-service/src/main/java/org/apache/polaris/service/context/RealmScopeContext.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.context;
+
+import jakarta.inject.Singleton;
+import java.lang.annotation.Annotation;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.polaris.core.context.CallContext;
+import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.core.context.RealmScope;
+import org.glassfish.hk2.api.ActiveDescriptor;
+import org.glassfish.hk2.api.Context;
+import org.glassfish.hk2.api.ServiceHandle;
+
+@Singleton
+public class RealmScopeContext implements Context<RealmScope> {
+ private final Map<String, Map<ActiveDescriptor<?>, Object>> contexts = new
ConcurrentHashMap<>();
+
+ @Override
+ public Class<? extends Annotation> getScope() {
+ return RealmScope.class;
+ }
+
+ @SuppressWarnings("unchecked")
+ @Override
+ public <U> U findOrCreate(ActiveDescriptor<U> activeDescriptor,
ServiceHandle<?> root) {
+ RealmContext realmContext =
CallContext.getCurrentContext().getRealmContext();
+ Map<ActiveDescriptor<?>, Object> contextMap =
+ contexts.computeIfAbsent(realmContext.getRealmIdentifier(), k -> new
ConcurrentHashMap<>());
+ return (U) contextMap.computeIfAbsent(activeDescriptor, k ->
activeDescriptor.create(root));
+ }
+
+ @Override
+ public boolean containsKey(ActiveDescriptor<?> descriptor) {
+ RealmContext realmContext =
CallContext.getCurrentContext().getRealmContext();
+ Map<ActiveDescriptor<?>, Object> contextMap =
+ contexts.computeIfAbsent(realmContext.getRealmIdentifier(), k -> new
HashMap<>());
+ return contextMap.containsKey(descriptor);
+ }
+
+ @Override
+ public void destroyOne(ActiveDescriptor<?> descriptor) {
+ RealmContext realmContext =
CallContext.getCurrentContext().getRealmContext();
+ Map<ActiveDescriptor<?>, Object> contextMap =
+ contexts.computeIfAbsent(realmContext.getRealmIdentifier(), k -> new
HashMap<>());
+ contextMap.remove(descriptor);
+ }
+
+ @Override
+ public boolean supportsNullCreation() {
+ return false;
+ }
+
+ @Override
+ public boolean isActive() {
+ return CallContext.getCurrentContext() != null
Review Comment:
Here we are using `CallContext.getCurrentContext()`, which internally relies
on thread-locals. This is NOT going to work well with CDI. We have already
opened an issue to raise awareness of that: #463. My long-term vision for
`CallContext` and alike is:
* `CallContext` and `PolarisCallContext` must disappear eventually;
* `RealmContext` can stay, and become request-scoped.
In the meanwhile, since removing `CallContext` is a large change, what I did
in the Quarkus PR is that I turned all 3 beans into request-scoped, and they
are `@Inject`-ed whenever required, avoiding `CallContext.getCurrentContext()`
as much as possible.
##########
polaris-service/src/main/java/org/apache/polaris/service/auth/JWTRSAKeyPairFactory.java:
##########
@@ -39,7 +39,7 @@ public TokenBroker apply(RealmContext realmContext) {
maxTokenGenerationInSeconds);
}
- @Override
+ @Inject
Review Comment:
setter -> constructor
##########
polaris-service/src/main/java/org/apache/polaris/service/auth/JWTSymmetricKeyFactory.java:
##########
@@ -68,7 +68,7 @@ public void setSecret(String secret) {
this.secret = secret;
}
- @Override
+ @Inject
public void setMetaStoreManagerFactory(MetaStoreManagerFactory
metaStoreManagerFactory) {
Review Comment:
setter -> constructor
##########
polaris-service/src/main/java/org/apache/polaris/service/auth/TestOAuth2ApiService.java:
##########
@@ -108,11 +108,8 @@ private String getPrincipalName(String clientId) {
}
}
- @Override
+ @Inject
public void setMetaStoreManagerFactory(MetaStoreManagerFactory
metaStoreManagerFactory) {
Review Comment:
setter -> constructor
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java:
##########
@@ -54,7 +54,7 @@ public abstract class
LocalPolarisMetaStoreManagerFactory<StoreType>
final Map<String, Supplier<PolarisMetaStoreSession>> sessionSupplierMap =
new HashMap<>();
protected final PolarisDiagnostics diagServices = new
PolarisDefaultDiagServiceImpl();
- protected PolarisStorageIntegrationProvider storageIntegration;
+ @Inject protected PolarisStorageIntegrationProvider storageIntegration;
Review Comment:
This should be removed from here (along with the setter below) because this
field is not used in this class.
Instead it should be injected on concrete subtypes that actually need it.
##########
polaris-service/src/main/java/org/apache/polaris/service/context/RealmScopeContext.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.context;
+
+import jakarta.inject.Singleton;
+import java.lang.annotation.Annotation;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.polaris.core.context.CallContext;
+import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.core.context.RealmScope;
+import org.glassfish.hk2.api.ActiveDescriptor;
+import org.glassfish.hk2.api.Context;
+import org.glassfish.hk2.api.ServiceHandle;
+
+@Singleton
+public class RealmScopeContext implements Context<RealmScope> {
+ private final Map<String, Map<ActiveDescriptor<?>, Object>> contexts = new
ConcurrentHashMap<>();
+
+ @Override
+ public Class<? extends Annotation> getScope() {
+ return RealmScope.class;
+ }
+
+ @SuppressWarnings("unchecked")
+ @Override
+ public <U> U findOrCreate(ActiveDescriptor<U> activeDescriptor,
ServiceHandle<?> root) {
Review Comment:
I think we are mixing concepts here, a request scoped bean should not
outlive the request. In the Quarkus PR the `RealmContext` is a request-scoped
bean, and one instance of it is created for each request. It two requests
target the same realm, they will get two distinct instances of `RealmContext`,
but each would have the same realm ID.
##########
polaris-service/src/main/java/org/apache/polaris/service/auth/DefaultOAuth2ApiService.java:
##########
@@ -39,8 +40,8 @@
* Default implementation of the {@link OAuth2ApiService} that generates a JWT
token for the client
* if the client secret matches.
*/
-@JsonTypeName("default")
-public class DefaultOAuth2ApiService implements OAuth2ApiService,
HasMetaStoreManagerFactory {
+@Named("default")
Review Comment:
I agree that `@Named` has some shortcomings. From [CDI spec
3.0](https://jakarta.ee/specifications/cdi/3.0/jakarta-cdi-spec-3.0#named_at_injection_point):
> The use of `@Named` as an injection point qualifier is not recommended,
except in the case of integration with legacy code that uses string-based names
to identify beans.
One of them is that the name is application-wide, so you cannot have two
beans named `@Named("default")`.
Instead, we could use some other qualifier. People generally use
`@Identifier` from Smallrye-Common library. The Quarkus PR is using that for
now.
This article explains the pros and cons of each annotation:
https://dev.to/yanev/unveiling-challenges-with-named-67p
##########
polaris-service/src/main/java/org/apache/polaris/service/auth/DefaultOAuth2ApiService.java:
##########
@@ -121,14 +122,14 @@ public Response getToken(
.build();
}
- @Override
+ @Inject
Review Comment:
Can we turn these setters into constructor injection? Also the
`HasMetaStoreManagerFactory` and other similar ones should be removed, they are
not needed in a CDI world.
##########
polaris-service/src/main/java/org/apache/polaris/service/persistence/cache/EntityCacheFactory.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.persistence.cache;
+
+import jakarta.inject.Inject;
+import org.apache.polaris.core.context.CallContext;
+import org.apache.polaris.core.context.RealmScope;
+import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
+import org.apache.polaris.core.persistence.cache.EntityCache;
+import org.glassfish.hk2.api.Factory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class EntityCacheFactory implements Factory<EntityCache> {
+ private static Logger LOGGER =
LoggerFactory.getLogger(EntityCacheFactory.class);
+ @Inject PolarisMetaStoreManager metaStoreManager;
+
+ @RealmScope
+ @Override
+ public EntityCache provide() {
+ LOGGER.debug(
+ "Creating new EntityCache instance for realm {}",
+
CallContext.getCurrentContext().getRealmContext().getRealmIdentifier());
Review Comment:
`RealmContext` should be injected, not retrieved from thread locals. That's
what `RealmEntityManagerFactory` does, cf.
`getOrCreateEntityManager(RealmContext)`.
##########
polaris-service/src/main/java/org/apache/polaris/service/context/RealmScopeContext.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.context;
+
+import jakarta.inject.Singleton;
+import java.lang.annotation.Annotation;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.polaris.core.context.CallContext;
+import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.core.context.RealmScope;
+import org.glassfish.hk2.api.ActiveDescriptor;
+import org.glassfish.hk2.api.Context;
+import org.glassfish.hk2.api.ServiceHandle;
+
+@Singleton
+public class RealmScopeContext implements Context<RealmScope> {
Review Comment:
So, I'm really, really not comfortable with this `RealmScopeContext`.
Speaking from experience, most custom scopes are hard to maintain, and can
introduce subtle bugs that are hard to chase. For instance, it's never easy to
know when to close a custom scope.
OTOH, most custom scoped-beans can actually be retrofitted to be
request-scoped beans with a bit of refactoring. This is imho much safer because
we don't have to care about when to open or when to close the scope.
In the Quarkus PR, `RealmContext` is a request-scoped bean, and that's
working fine.
##########
polaris-service/src/main/java/org/apache/polaris/service/config/RealmEntityManagerFactory.java:
##########
@@ -18,29 +18,39 @@
*/
package org.apache.polaris.service.config;
+import jakarta.inject.Inject;
+import jakarta.inject.Provider;
+import java.util.HashMap;
import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.core.context.RealmScope;
import org.apache.polaris.core.persistence.MetaStoreManagerFactory;
import org.apache.polaris.core.persistence.PolarisEntityManager;
+import org.apache.polaris.core.persistence.cache.EntityCache;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/** Gets or creates PolarisEntityManager instances based on config values and
RealmContext. */
+@RealmScope
public class RealmEntityManagerFactory {
Review Comment:
I disagree here, this is clearly an application-scoped bean, holding
long-lived instances of `PolarisEntityManager` keyed by realm id.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegrationProviderImpl.java:
##########
@@ -16,29 +16,26 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.service.storage;
+package org.apache.polaris.core.storage;
Review Comment:
Why move an implementation class to core?
##########
polaris-service/src/main/java/org/apache/polaris/service/persistence/InMemoryPolarisMetaStoreManagerFactory.java:
##########
@@ -35,9 +35,10 @@
import org.apache.polaris.core.persistence.PolarisTreeMapMetaStoreSessionImpl;
import org.apache.polaris.core.persistence.PolarisTreeMapStore;
-@JsonTypeName("in-memory")
+@Named("in-memory")
+@Singleton
Review Comment:
We have to decide between `@Singleton` and `@ApplicationScoped` – unless we
have a good reason to use the former, the latter is preferred.
##########
polaris-service/src/main/java/org/apache/polaris/service/BootstrapRealmsCommand.java:
##########
@@ -48,17 +46,11 @@ protected void run(
Bootstrap<PolarisApplicationConfig> bootstrap,
Namespace namespace,
PolarisApplicationConfig configuration) {
- MetaStoreManagerFactory metaStoreManagerFactory =
configuration.getMetaStoreManagerFactory();
+ MetaStoreManagerFactory metaStoreManagerFactory =
Review Comment:
Commands (boostrap/purge) are going to be fun to adapt to CDI. I suggest we
look into them later but my feeling is that we would need to move them to
separate modules.
##########
polaris-service/src/main/java/org/apache/polaris/service/persistence/cache/EntityCacheFactory.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.persistence.cache;
+
+import jakarta.inject.Inject;
+import org.apache.polaris.core.context.CallContext;
+import org.apache.polaris.core.context.RealmScope;
+import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
+import org.apache.polaris.core.persistence.cache.EntityCache;
+import org.glassfish.hk2.api.Factory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class EntityCacheFactory implements Factory<EntityCache> {
+ private static Logger LOGGER =
LoggerFactory.getLogger(EntityCacheFactory.class);
+ @Inject PolarisMetaStoreManager metaStoreManager;
+
+ @RealmScope
Review Comment:
Again I think it's much easier to turn this bean into application-scoped and
have it hold a map of entity caches keyed by realm id, like
`RealmEntityManagerFactory`.
##########
server-templates/api.mustache:
##########
@@ -83,6 +85,7 @@ public class {{classname}} {
private final {{classname}}Service service;
+ @Inject
Review Comment:
If you turn `RealmContext` into request-scoped, a nice benefit that comes
for free is that you could inject it directly into the REST resources generated
from the open-api spec. This way, the realm is immediately available and
injected everywhere. E.g. the /config endpoint could become:
```java
public Response getConfig(
@QueryParam("warehouse") String warehouse,
@Context RealmContext realmContext,
@Context SecurityContext securityContext) {
return service.getConfig(warehouse, realmContext, securityContext);
}
```
##########
polaris-service/src/main/java/org/apache/polaris/service/context/DefaultContextResolver.java:
##########
@@ -166,7 +165,7 @@ private static Map<String, String>
parseBearerTokenAsKvPairs(Map<String, String>
return parsedProperties;
}
- @Override
+ @Inject
Review Comment:
setter -> constructor
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisEntityManager.java:
##########
@@ -51,11 +51,14 @@ public class PolarisEntityManager {
/**
* @param metaStoreManager the metastore manager for the current realm
* @param credentialCache the storage credential cache for the current realm
+ * @param entityCache the entity cache
*/
public PolarisEntityManager(
- PolarisMetaStoreManager metaStoreManager, StorageCredentialCache
credentialCache) {
+ PolarisMetaStoreManager metaStoreManager,
Review Comment:
I asked myself this question as well when doing the Quarkus port. But in the
end I left `PolarisEntityManager` as a non-CDI class. There might be room for
improvement around here.
--
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]
