Skip to content

Working impl of HK2 dependency injection #493

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 27 commits into from
Dec 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
795416d
Working impl of HK2 dependency injection
collado-mike Nov 6, 2024
8361fd8
Add RealmScope and make EntityCache and PolarisMetaStoreManager realm…
collado-mike Nov 7, 2024
578ff76
Remove dependency on HK2 from core module
collado-mike Nov 25, 2024
7df1038
Fix EntityCache to work with RealmScope
collado-mike Nov 26, 2024
ab1e19d
fix TimedApplicationEventListener.class
collado-mike Nov 26, 2024
ccb710f
Add integration test to validate EntityCache in RealmScope
collado-mike Nov 26, 2024
e01a66a
Added comments on PolarisApplication startup
collado-mike Nov 26, 2024
080113e
Update HK2 setup to use PolarisApplicationConfig as interface to serv…
collado-mike Nov 27, 2024
c4950ae
Fixed RealmEntityManagerFactory to use EntityCache provider
collado-mike Nov 28, 2024
9763f0b
Update Dockerfile to use entrypoint and fix vended-credentials delega…
collado-mike Nov 28, 2024
46abf02
fix spotless
collado-mike Nov 28, 2024
89d2e2b
Update polaris-core/src/main/java/org/apache/polaris/core/context/Rea…
collado-mike Dec 2, 2024
025c992
Removed Jackson type annotations from interfaces
collado-mike Dec 3, 2024
9e09e03
Address PR comments
collado-mike Dec 3, 2024
88b960e
Update build versions to include jakarta.inject
collado-mike Dec 3, 2024
73ca1ed
Removed unnecessary Discoverable service files
collado-mike Dec 3, 2024
2f60a7b
Fixes for rebase on main
collado-mike Dec 3, 2024
43640c3
Update Jersey ServiceLocator to use bootstrapped locator as parent
collado-mike Dec 4, 2024
a3d6141
Addressed more PR comments
sfc-gh-mcollado Dec 5, 2024
5b2182d
Update RealmScopeContext to use request-scoped RealmContext bean
sfc-gh-mcollado Dec 6, 2024
25e130c
Update CDI impls to use @Identifier and replaced all setter injection…
sfc-gh-mcollado Dec 6, 2024
e7928fc
Fix helm test to match expectation
sfc-gh-mcollado Dec 6, 2024
492f3bf
Revert PolarisStorageIntegrationProviderImpl back to service module
sfc-gh-mcollado Dec 6, 2024
705221c
Fix missing project mentions in LICENSE
sfc-gh-mcollado Dec 6, 2024
2587138
Fix lint issue in helm values
sfc-gh-mcollado Dec 6, 2024
2190330
Add logic in build to merge multiple hks-locator files
sfc-gh-mcollado Dec 6, 2024
30a404a
Merge branch 'main' into mcollado-hk2-di
collado-mike Dec 9, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions LICENSE
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,14 @@ io.prometheus:prometheus-metrics-exposition-formats
io.prometheus:prometheus-metrics-model
io.prometheus:prometheus-metrics-shaded-protobuf
io.prometheus:prometheus-metrics-tracer-common
io.smallrye.common:smallrye-common-annotation
io.smallrye.common:smallrye-common-classloader
io.smallrye.common:smallrye-common-constraint
io.smallrye.common:smallrye-common-expression
io.smallrye.common:smallrye-common-function
io.smallrye.config:smallrye-config
io.smallrye.config:smallrye-config-common
io.smallrye.config:smallrye-config-core
io.swagger:swagger-annotations
io.swagger:swagger-core
io.swagger:swagger-jaxrs
Expand Down Expand Up @@ -446,6 +454,7 @@ org.eclipse.jetty:jetty-servlets
org.eclipse.jetty:jetty-util
org.eclipse.jetty:jetty-webapp
org.eclipse.jetty:jetty-xml
org.eclipse.microprofile.config:microprofile-config-api
org.glassfish.jersey.containers:jersey-container-servlet
org.glassfish.jersey.containers:jersey-container-servlet-core
org.glassfish.jersey.core:jersey-client
Expand Down
2 changes: 2 additions & 0 deletions extension/persistence/eclipselink/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ dependencies {
implementation(project(":polaris-jpa-model"))
implementation(libs.eclipselink)
implementation(platform(libs.dropwizard.bom))
implementation(libs.jakarta.inject.api)
implementation(libs.smallrye)
implementation("io.dropwizard:dropwizard-jackson")
val eclipseLinkDeps: String? = project.findProperty("eclipseLinkDeps") as String?
eclipseLinkDeps?.let {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,29 +19,32 @@
package org.apache.polaris.extension.persistence.impl.eclipselink;

import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonTypeName;
import io.dropwizard.jackson.Discoverable;
import io.smallrye.common.annotation.Identifier;
import jakarta.annotation.Nonnull;
import jakarta.inject.Inject;
import org.apache.polaris.core.PolarisDiagnostics;
import org.apache.polaris.core.context.RealmContext;
import org.apache.polaris.core.persistence.LocalPolarisMetaStoreManagerFactory;
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
import org.apache.polaris.core.persistence.PolarisMetaStoreSession;
import org.apache.polaris.core.storage.PolarisStorageIntegrationProvider;

/**
* The implementation of Configuration interface for configuring the {@link PolarisMetaStoreManager}
* using an EclipseLink based meta store to store and retrieve all Polaris metadata. It can be
* configured through persistence.xml to use supported RDBMS as the meta store.
*/
@JsonTypeName("eclipse-link")
@Identifier("eclipse-link")
public class EclipseLinkPolarisMetaStoreManagerFactory
extends LocalPolarisMetaStoreManagerFactory<PolarisEclipseLinkStore> implements Discoverable {
extends LocalPolarisMetaStoreManagerFactory<PolarisEclipseLinkStore> {
@JsonProperty("conf-file")
private String confFile;

@JsonProperty("persistence-unit")
private String persistenceUnitName;

@Inject protected PolarisStorageIntegrationProvider storageIntegration;

@Override
protected PolarisEclipseLinkStore createBackingStore(@Nonnull PolarisDiagnostics diagnostics) {
return new PolarisEclipseLinkStore(diagnostics);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,8 @@
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.polaris.core.monitor;
[org.apache.polaris.extension.persistence.impl.eclipselink.EclipseLinkPolarisMetaStoreManagerFactory]S
contract={org.apache.polaris.core.persistence.MetaStoreManagerFactory}
name=eclipse-link
qualifier={io.smallrye.common.annotation.Identifier}

/** Allows setting a configured instance of {@link PolarisMetricRegistry} */
public interface MetricRegistryAware {
void setMetricRegistry(PolarisMetricRegistry metricRegistry);
}

This file was deleted.

2 changes: 2 additions & 0 deletions gradle/libs.versions.toml
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ hadoop-hdfs-client = { module = "org.apache.hadoop:hadoop-hdfs-client", version.
iceberg-bom = { module = "org.apache.iceberg:iceberg-bom", version.ref = "iceberg" }
jackson-bom = { module = "com.fasterxml.jackson:jackson-bom", version = "2.17.2" }
jakarta-annotation-api = { module = "jakarta.annotation:jakarta.annotation-api", version = "3.0.0" }
jakarta-inject-api = { module = "jakarta.inject:jakarta.inject-api", version = "2.0.1" }
jakarta-validation-api = { module = "jakarta.validation:jakarta.validation-api", version = "3.1.0" }
jakarta-persistence-api = { module = "jakarta.persistence:jakarta.persistence-api", version = "3.1.0" }
javax-annotation-api = { module = "javax.annotation:javax.annotation-api", version = "1.3.2" }
Expand All @@ -71,6 +72,7 @@ swagger-annotations = { module = "io.swagger:swagger-annotations", version.ref =
swagger-jaxrs = { module = "io.swagger:swagger-jaxrs", version.ref = "swagger" }
testcontainers-bom = { module = "org.testcontainers:testcontainers-bom", version = "1.20.3" }
threeten-extra = { module = "org.threeten:threeten-extra", version = "1.8.0" }
smallrye = { module = "io.smallrye.config:smallrye-config", version = "3.10.2" }

[plugins]
openapi-generator = { id = "org.openapi.generator", version = "7.6.0" }
Expand Down
18 changes: 6 additions & 12 deletions helm/polaris/tests/configmap_test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -184,14 +184,11 @@ tests:
factoryType: default
oauth2:
type: default
tokenBroker:
type: symmetric-key
secret: polaris
tokenBroker:
type: symmetric-key
secret: polaris
authenticator:
class: org.apache.polaris.service.auth.DefaultPolarisAuthenticator
tokenBroker:
type: symmetric-key
secret: polaris
cors:
allowed-origins:
- http://localhost:8080
Expand Down Expand Up @@ -228,9 +225,6 @@ tests:
polaris-server.yml: |-
authenticator:
class: org.apache.polaris.service.auth.DefaultPolarisAuthenticator
tokenBroker:
secret: polaris
type: symmetric-key
callContextResolver:
type: default
cors:
Expand Down Expand Up @@ -274,9 +268,6 @@ tests:
persistence-unit: polaris
type: eclipse-link
oauth2:
tokenBroker:
secret: polaris
type: symmetric-key
type: default
rateLimiter:
type: no-op
Expand All @@ -294,3 +285,6 @@ tests:
requestLog:
appenders:
- type: console
tokenBroker:
secret: polaris
type: symmetric-key
6 changes: 3 additions & 3 deletions helm/polaris/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -321,9 +321,9 @@ polarisServerConfig:
oauth2:
type: test
# type: default # - uncomment to support Auth0 JWT tokens
# tokenBroker:
# type: symmetric-key
# secret: polaris
# tokenBroker:
# type: symmetric-key
# secret: polaris

authenticator:
class: org.apache.polaris.service.auth.TestInlineBearerTokenPolarisAuthenticator
Expand Down
2 changes: 2 additions & 0 deletions polaris-core/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,9 @@ dependencies {
implementation(libs.javax.inject)
implementation(libs.swagger.annotations)
implementation(libs.swagger.jaxrs)
implementation(libs.jakarta.inject.api)
implementation(libs.jakarta.validation.api)
implementation(libs.smallrye)

implementation("org.apache.iceberg:iceberg-aws")
implementation(platform(libs.awssdk.bom))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@
import com.google.common.collect.SetMultimap;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import jakarta.inject.Inject;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -460,6 +461,7 @@ public class PolarisAuthorizerImpl implements PolarisAuthorizer {

private final PolarisConfigurationStore featureConfig;

@Inject
public PolarisAuthorizerImpl(PolarisConfigurationStore featureConfig) {
this.featureConfig = featureConfig;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,16 @@
* 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.Documented;
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
@Documented
@Retention(java.lang.annotation.RetentionPolicy.RUNTIME)
@Target({ElementType.TYPE, ElementType.METHOD})
public @interface RealmScoped {}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import io.micrometer.core.instrument.binder.jvm.JvmMemoryMetrics;
import io.micrometer.core.instrument.binder.jvm.JvmThreadMetrics;
import io.micrometer.core.instrument.binder.system.ProcessorMetrics;
import jakarta.inject.Inject;
import java.lang.reflect.Method;
import java.util.Collections;
import java.util.concurrent.ConcurrentHashMap;
Expand Down Expand Up @@ -68,6 +69,7 @@ public class PolarisMetricRegistry {
public static final String SUFFIX_ERROR = ".error";
public static final String SUFFIX_REALM = ".realm";

@Inject
public PolarisMetricRegistry(MeterRegistry meterRegistry) {
this.meterRegistry = meterRegistry;
new ClassLoaderMetrics().bindTo(meterRegistry);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@
import org.apache.polaris.core.entity.PolarisEntitySubType;
import org.apache.polaris.core.entity.PolarisEntityType;
import org.apache.polaris.core.entity.PolarisPrincipalSecrets;
import org.apache.polaris.core.monitor.PolarisMetricRegistry;
import org.apache.polaris.core.storage.PolarisStorageIntegrationProvider;
import org.apache.polaris.core.storage.cache.StorageCredentialCache;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -54,8 +52,6 @@ public abstract class LocalPolarisMetaStoreManagerFactory<StoreType>
final Map<String, Supplier<PolarisMetaStoreSession>> sessionSupplierMap = new HashMap<>();
protected final PolarisDiagnostics diagServices = new PolarisDefaultDiagServiceImpl();

protected PolarisStorageIntegrationProvider storageIntegration;

private static final Logger LOGGER =
LoggerFactory.getLogger(LocalPolarisMetaStoreManagerFactory.class);

Expand Down Expand Up @@ -157,16 +153,6 @@ public synchronized StorageCredentialCache getOrCreateStorageCredentialCache(
return storageCredentialCacheMap.get(realmContext.getRealmIdentifier());
}

@Override
public void setMetricRegistry(PolarisMetricRegistry metricRegistry) {
// no-op
}

@Override
public void setStorageIntegrationProvider(PolarisStorageIntegrationProvider storageIntegration) {
this.storageIntegration = storageIntegration;
}

/**
* This method bootstraps service for a given realm: i.e. creates all the needed entities in the
* metastore and creates a root service principal. After that we rotate the root principal
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,17 @@
*/
package org.apache.polaris.core.persistence;

import com.fasterxml.jackson.annotation.JsonTypeInfo;
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")
public interface MetaStoreManagerFactory {

PolarisMetaStoreManager getOrCreateMetaStoreManager(RealmContext realmContext);
Expand All @@ -41,10 +37,6 @@ public interface MetaStoreManagerFactory {

StorageCredentialCache getOrCreateStorageCredentialCache(RealmContext realmContext);

void setStorageIntegrationProvider(PolarisStorageIntegrationProvider storageIntegrationProvider);

void setMetricRegistry(PolarisMetricRegistry metricRegistry);

Map<String, PrincipalSecretsResult> bootstrapRealms(List<String> realms);

/** Purge all metadata for the realms provided */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like PolarisEntityManager should become a CDI bean as well and get the instances injected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would effectively eliminate the need for RealmEntityManagerFactory - which is fine, but it means the callers need to be made either realm-scoped or request-scoped

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Callers should be request-scoped, no?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, the PolarisEntityManager is really just a factory for the Resolver now. That whole process is something I think should be a lower-level concern, hidden under the higher level interfaces. We've discussed a "unit of work" approach before and I think that would be a good layer to encapsulate the Resolver work. At that point, there is no need for the PolarisEntityManager and the Resolver itself would be managed by the CDI container, but hidden from application logic.

StorageCredentialCache credentialCache,
EntityCache entityCache) {
this.metaStoreManager = metaStoreManager;
this.entityCache = new EntityCache(metaStoreManager);
this.entityCache = entityCache;
this.credentialCache = credentialCache;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,20 @@
import com.github.benmanes.caffeine.cache.RemovalListener;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import jakarta.inject.Inject;
import java.util.AbstractMap;
import java.util.List;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;
import org.apache.polaris.core.PolarisCallContext;
import org.apache.polaris.core.context.RealmScoped;
import org.apache.polaris.core.entity.PolarisBaseEntity;
import org.apache.polaris.core.entity.PolarisEntityType;
import org.apache.polaris.core.entity.PolarisGrantRecord;
import org.apache.polaris.core.persistence.cache.PolarisRemoteCache.CachedEntryResult;

/** The entity cache, can be private or shared */
@RealmScoped
public class EntityCache {

// cache mode
Expand All @@ -53,6 +56,7 @@ public class EntityCache {
*
* @param polarisRemoteCache the meta store manager implementation
*/
@Inject
public EntityCache(@Nonnull PolarisRemoteCache polarisRemoteCache) {

// by name cache
Expand Down
18 changes: 10 additions & 8 deletions polaris-server.yml
Original file line number Diff line number Diff line change
Expand Up @@ -93,17 +93,19 @@ io:
# TODO - avoid duplicating token broker config
oauth2:
type: test
# type: default # - uncomment to support Auth0 JWT tokens
# tokenBroker:
# type: symmetric-key
# secret: polaris

authenticator:
class: org.apache.polaris.service.auth.TestInlineBearerTokenPolarisAuthenticator

# - uncomment to support Auth0 JWT tokens
#oauth2:
# type: default
#authenticator:
# class: org.apache.polaris.service.auth.DefaultPolarisAuthenticator # - uncomment to support Auth0 JWT tokens
# tokenBroker:
# type: symmetric-key
# secret: polaris
#
#tokenBroker:
# type: symmetric-key
# secret: polaris

cors:
allowed-origins:
Expand Down Expand Up @@ -133,7 +135,7 @@ logging:
# Logger-specific levels.
loggers:
org.apache.iceberg.rest: DEBUG
org.apache.polaris: DEBUG
org.apache.polaris: INFO

appenders:

Expand Down
Loading
Loading