snazy commented on code in PR #493:
URL: https://github.com/apache/polaris/pull/493#discussion_r1865739961
##########
polaris-core/build.gradle.kts:
##########
@@ -72,6 +68,7 @@ dependencies {
implementation(libs.swagger.annotations)
implementation(libs.swagger.jaxrs)
implementation(libs.jakarta.validation.api)
+ implementation("jakarta.inject:jakarta.inject-api:2.0.1")
Review Comment:
Should be managed via the version catalog.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##########
@@ -19,33 +19,26 @@
package org.apache.polaris.core.persistence;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
-import io.dropwizard.jackson.Discoverable;
import java.util.List;
import java.util.Map;
import java.util.function.Supplier;
import
org.apache.polaris.core.auth.PolarisSecretsManager.PrincipalSecretsResult;
import org.apache.polaris.core.context.RealmContext;
-import org.apache.polaris.core.monitor.PolarisMetricRegistry;
-import org.apache.polaris.core.storage.PolarisStorageIntegrationProvider;
import org.apache.polaris.core.storage.cache.StorageCredentialCache;
/**
* Configuration interface for configuring the {@link PolarisMetaStoreManager}
via Dropwizard
* configuration
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY,
property = "type")
Review Comment:
While this and #469 aim to migrate to "proper CDI", the use of DI via
Jackson should be removed as well.
##########
polaris-service/src/main/java/org/apache/polaris/service/PolarisApplication.java:
##########
@@ -151,87 +173,195 @@ public void
initialize(Bootstrap<PolarisApplicationConfig> bootstrap) {
bootstrap.addCommand(new BootstrapRealmsCommand());
bootstrap.addCommand(new PurgeRealmsCommand());
- }
+ serviceLocator = ServiceLocatorUtilities.createAndPopulateServiceLocator();
+ SimpleValueInstantiators instantiators = new SimpleValueInstantiators();
+ ObjectMapper objectMapper = bootstrap.getObjectMapper();
- @Override
- public void run(PolarisApplicationConfig configuration, Environment
environment) {
- MetaStoreManagerFactory metaStoreManagerFactory =
configuration.getMetaStoreManagerFactory();
-
- metaStoreManagerFactory.setStorageIntegrationProvider(
- new PolarisStorageIntegrationProviderImpl(
- () -> {
- StsClientBuilder stsClientBuilder = StsClient.builder();
- AwsCredentialsProvider awsCredentialsProvider =
configuration.credentialsProvider();
- if (awsCredentialsProvider != null) {
- stsClientBuilder.credentialsProvider(awsCredentialsProvider);
+ // Register the PolarisApplicationConfig class with the ServiceLocator so
that we can inject
+ // instances of the configuration or certain of the configuration fields
into other classes.
+ // The configuration's postConstruct method registers all the configured
fields as factories
+ // so that they are used for DI.
+ ServiceLocatorUtilities.addClasses(serviceLocator,
PolarisApplicationConfig.class);
+ TypeFactory typeFactory = TypeFactory.defaultInstance();
+ instantiators.addValueInstantiator(
+ PolarisApplicationConfig.class,
+ new ServiceLocatorValueInstantiator(
+ objectMapper.getDeserializationConfig(),
+ typeFactory.constructType(PolarisApplicationConfig.class),
+ serviceLocator));
+
+ // Use the default ServiceLocator to discover the implementations of
various contract providers
+ // and register them as subtypes with the ObjectMapper. This allows
Jackson to discover the
+ // various implementations and use the type annotations to determine which
instance to use when
+ // parsing the YAML configuration.
+ serviceLocator
+ .getDescriptors((c) -> true)
+ .forEach(
+ descriptor -> {
+ try {
+ Class<?> klazz =
+ PolarisApplication.class
+ .getClassLoader()
+ .loadClass(descriptor.getImplementation());
+ String name = descriptor.getName();
+ if (name == null) {
+ objectMapper.registerSubtypes(klazz);
+ } else {
+ objectMapper.registerSubtypes(new NamedType(klazz, name));
+ }
+ } catch (ClassNotFoundException e) {
+ LOGGER.error("Error loading class {}",
descriptor.getImplementation(), e);
+ throw new RuntimeException("Unable to start Polaris
application");
}
- return stsClientBuilder.build();
- },
- configuration.getGcpCredentialsProvider()));
+ });
- PolarisMetricRegistry polarisMetricRegistry =
- new PolarisMetricRegistry(new
PrometheusMeterRegistry(PrometheusConfig.DEFAULT));
- metaStoreManagerFactory.setMetricRegistry(polarisMetricRegistry);
+ ServiceLocatorUtilities.addClasses(serviceLocator,
PolarisMetricRegistry.class);
+ ServiceLocatorUtilities.bind(
+ serviceLocator,
+ new AbstractBinder() {
+ @Override
+ protected void configure() {
+ bind(new
PrometheusMeterRegistry(PrometheusConfig.DEFAULT)).to(MeterRegistry.class);
+ }
+ });
+ SimpleModule module = new SimpleModule();
+ module.setValueInstantiators(instantiators);
+ module.setMixInAnnotation(Authenticator.class, NamedAuthenticator.class);
+ objectMapper.registerModule(module);
+ }
- OpenTelemetry openTelemetry = setupTracing();
- if (metaStoreManagerFactory instanceof OpenTelemetryAware otAware) {
- otAware.setOpenTelemetry(openTelemetry);
+ /**
+ * Value instantiator that uses the ServiceLocator to create instances of
the various service
+ * types
+ */
+ private static class ServiceLocatorValueInstantiator extends
StdValueInstantiator {
+ private final ServiceLocator serviceLocator;
+
+ public ServiceLocatorValueInstantiator(
+ DeserializationConfig config, JavaType valueType, ServiceLocator
serviceLocator) {
+ super(config, valueType);
+ this.serviceLocator = serviceLocator;
}
- PolarisConfigurationStore configurationStore =
configuration.getConfigurationStore();
- if (metaStoreManagerFactory instanceof ConfigurationStoreAware) {
- ((ConfigurationStoreAware)
metaStoreManagerFactory).setConfigurationStore(configurationStore);
+
+ @Override
+ public boolean canCreateUsingDefault() {
+ return true;
}
- RealmEntityManagerFactory entityManagerFactory =
- new RealmEntityManagerFactory(metaStoreManagerFactory);
- CallContextResolver callContextResolver =
configuration.getCallContextResolver();
- callContextResolver.setMetaStoreManagerFactory(metaStoreManagerFactory);
- if (callContextResolver instanceof ConfigurationStoreAware csa) {
- csa.setConfigurationStore(configurationStore);
+
+ @Override
+ public boolean canInstantiate() {
+ return true;
}
- RealmContextResolver realmContextResolver =
configuration.getRealmContextResolver();
- realmContextResolver.setMetaStoreManagerFactory(metaStoreManagerFactory);
+ @Override
+ public Object createUsingDefault(DeserializationContext ctxt) throws
IOException {
+ return ServiceLocatorUtilities.findOrCreateService(serviceLocator,
getValueClass());
+ }
+ }
+
+ /**
+ * Authenticator uses the java Class name in the YAML configuration. Since
the Authenticator
+ * interface is owned by Dropwizard, we need to use a MixIn to tell Jackson
what field to use to
+ * discover the implementation from the configuration.
+ */
+ @JsonTypeInfo(use = JsonTypeInfo.Id.CLASS, property = "class")
+ static final class NamedAuthenticator {}
+
+ @Override
+ public void run(PolarisApplicationConfig configuration, Environment
environment) {
+ OpenTelemetry openTelemetry = setupTracing();
+ PolarisMetricRegistry polarisMetricRegistry =
+ configuration.findService(PolarisMetricRegistry.class);
+
+ MetaStoreManagerFactory metaStoreManagerFactory =
+ configuration.findService(MetaStoreManagerFactory.class);
+
+ // Use the PolarisApplicationConfig to register dependencies in the Jersey
resource
+ // configuration. This uses a different ServiceLocator from the one in the
bootstrap step
+ environment.jersey().register(configuration.binder());
+ environment
+ .jersey()
+ .register(
+ new AbstractBinder() {
+ @Override
+ protected void configure() {
+ bind(RealmScopeContext.class)
Review Comment:
Looks like there's a lot of manual coding needed for hk2, which feels like
it makes things complicated, when we eventually have a plugin/extension
mechanism.
##########
polaris-service/src/main/java/org/apache/polaris/service/PolarisApplication.java:
##########
@@ -412,4 +533,31 @@ public void doFilter(ServletRequest request,
ServletResponse response, FilterCha
}
}
}
+
+ private static class PolarisMetaStoreManagerFactory implements
Factory<PolarisMetaStoreManager> {
Review Comment:
Should be replaced with `@Produces` and `@Disposes`
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java:
##########
@@ -157,12 +157,7 @@ public synchronized StorageCredentialCache
getOrCreateStorageCredentialCache(
return storageCredentialCacheMap.get(realmContext.getRealmIdentifier());
}
- @Override
- public void setMetricRegistry(PolarisMetricRegistry metricRegistry) {
- // no-op
- }
-
- @Override
+ @Inject
Review Comment:
Why not just add `@Inject` to the field?
##########
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:
Why not `@Inject` these?
##########
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:
Feels like `PolarisEntityManager` should become a CDI bean as well and get
the instances injected.
##########
polaris-service/src/main/java/org/apache/polaris/service/config/PolarisApplicationConfig.java:
##########
@@ -193,15 +348,15 @@ public long getMaxRequestBodyBytes() {
return maxRequestBodyBytes;
}
- public PolarisConfigurationStore getConfigurationStore() {
+ private PolarisConfigurationStore getConfigurationStore() {
Review Comment:
Note: this is not a getter, but creates a new instance.
##########
regtests/Dockerfile:
##########
@@ -54,4 +54,4 @@ USER root
RUN chmod -R go+rwx /home/spark/regtests
USER spark
-CMD ["./run.sh"]
+ENTRYPOINT ["./run.sh"]
Review Comment:
Change to this PR?
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegrationProvider.java:
##########
@@ -18,12 +18,17 @@
*/
package org.apache.polaris.core.storage;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
import org.jetbrains.annotations.Nullable;
/**
* Factory interface that knows how to construct a {@link
PolarisStorageIntegration} given a {@link
* PolarisStorageConfigurationInfo}.
*/
+@JsonTypeInfo(
Review Comment:
Same comment as above, let's not rely on Jackson for DI.
##########
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:
`@Named` has a couple drawbacks.
Using specialized `@Qualifier`s using `AnnotationLiteral`s to select the
configured type via `Instance.select()` is the most flexible but also the
safest way.
##########
polaris-core/src/main/java/org/apache/polaris/core/context/RealmScope.java:
##########
@@ -16,12 +16,14 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.service.config;
+package org.apache.polaris.core.context;
-import org.apache.polaris.core.PolarisConfigurationStore;
+import jakarta.inject.Scope;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.Target;
-/** Interface allows injection of a {@link PolarisConfigurationStore} */
-public interface ConfigurationStoreAware {
-
- void setConfigurationStore(PolarisConfigurationStore configurationStore);
-}
+@Scope
+@Retention(java.lang.annotation.RetentionPolicy.RUNTIME)
+@Target({ElementType.TYPE, ElementType.METHOD})
Review Comment:
```suggestion
@Target({ElementType.TYPE, ElementType.METHOD, ElementType.FIELD})
```
##########
polaris-core/src/main/java/org/apache/polaris/core/context/RealmScope.java:
##########
@@ -16,12 +16,14 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.service.config;
+package org.apache.polaris.core.context;
-import org.apache.polaris.core.PolarisConfigurationStore;
+import jakarta.inject.Scope;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.Target;
-/** Interface allows injection of a {@link PolarisConfigurationStore} */
-public interface ConfigurationStoreAware {
-
- void setConfigurationStore(PolarisConfigurationStore configurationStore);
-}
+@Scope
+@Retention(java.lang.annotation.RetentionPolicy.RUNTIME)
+@Target({ElementType.TYPE, ElementType.METHOD})
+public @interface RealmScope {}
Review Comment:
```suggestion
public @interface RealmScope extends AlterableContext {}
```
And have functionality to perform cleanup when a realm is removed.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCache.java:
##########
@@ -34,6 +36,7 @@
import org.jetbrains.annotations.Nullable;
/** The entity cache, can be private or shared */
+@RealmScope
Review Comment:
IMO a cache should leverage the available/configured resources of the JVM in
a way that the resources are used most efficiently but also cannot exceed the
available heap.
Here the cache is scoped to a realm, where each realm (number of realms is
unbounded) permanently allocates JVM resources.
Not related to this PR, but something that should be fixed.
##########
polaris-core/src/main/java/org/apache/polaris/core/context/RealmScope.java:
##########
@@ -16,12 +16,14 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.service.config;
+package org.apache.polaris.core.context;
-import org.apache.polaris.core.PolarisConfigurationStore;
+import jakarta.inject.Scope;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.Target;
-/** Interface allows injection of a {@link PolarisConfigurationStore} */
-public interface ConfigurationStoreAware {
-
- void setConfigurationStore(PolarisConfigurationStore configurationStore);
-}
+@Scope
Review Comment:
```suggestion
@Scope
@Documented
```
##########
polaris-service/src/main/java/org/apache/polaris/service/tracing/TracingFilter.java:
##########
@@ -53,6 +54,7 @@ public class TracingFilter implements Filter {
private static final Logger LOGGER =
LoggerFactory.getLogger(TracingFilter.class);
private final OpenTelemetry openTelemetry;
+ @Inject
Review Comment:
Generally, the CDI spec demands a public no-arg constructor - some
frameworks do not require it though.
##########
polaris-service/src/main/resources/META-INF/hk2-locator/default:
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.
+ */
+[org.apache.polaris.service.auth.DefaultPolarisAuthenticator]S
Review Comment:
Feels like this hk2 specific way to provide instances should be replaced
with CDI producers?
##########
polaris-service/src/main/java/org/apache/polaris/service/persistence/InMemoryPolarisMetaStoreManagerFactory.java:
##########
@@ -35,6 +37,8 @@
import org.jetbrains.annotations.NotNull;
@JsonTypeName("in-memory")
+@Named("in-memory")
Review Comment:
Here there's CDI added, but also Jackson used as a CDI-ish mechanism.
In CDI, this is a `@Singleton`, but what about Jackson??
--
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]
