Skip to content
Merged
Show file tree
Hide file tree
Changes from 18 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
1 change: 1 addition & 0 deletions extension/persistence/eclipselink/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ dependencies {
implementation(libs.eclipselink)
implementation(platform(libs.dropwizard.bom))
implementation("io.dropwizard:dropwizard-jackson")
implementation("org.glassfish.hk2:hk2-api:3.0.6")
val eclipseLinkDeps: String? = project.findProperty("eclipseLinkDeps") as String?
eclipseLinkDeps?.let {
val dependenciesList = it.split(",")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@
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 jakarta.annotation.Nonnull;
import jakarta.inject.Named;
import org.apache.polaris.core.PolarisDiagnostics;
import org.apache.polaris.core.context.RealmContext;
import org.apache.polaris.core.persistence.LocalPolarisMetaStoreManagerFactory;
Expand All @@ -33,7 +33,7 @@
* 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")
@Named("eclipse-link")
public class EclipseLinkPolarisMetaStoreManagerFactory
extends LocalPolarisMetaStoreManagerFactory<PolarisEclipseLinkStore> implements Discoverable {
@JsonProperty("conf-file")
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={jakarta.inject.Named}

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

This file was deleted.

1 change: 1 addition & 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 Down
1 change: 1 addition & 0 deletions polaris-core/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ dependencies {
implementation(libs.javax.inject)
implementation(libs.swagger.annotations)
implementation(libs.swagger.jaxrs)
implementation(libs.jakarta.inject.api)
implementation(libs.jakarta.validation.api)

implementation("org.apache.iceberg:iceberg-aws")
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,11 +16,16 @@
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.polaris.service.tracing;
package org.apache.polaris.core.context;

import io.opentelemetry.api.OpenTelemetry;
import jakarta.inject.Scope;
import java.lang.annotation.Documented;
import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.Target;

/** Allows setting a configured instance of {@link OpenTelemetry} */
public interface OpenTelemetryAware {
void setOpenTelemetry(OpenTelemetry openTelemetry);
}
@Scope
@Documented
@Retention(java.lang.annotation.RetentionPolicy.RUNTIME)
@Target({ElementType.TYPE, ElementType.METHOD})
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Target({ElementType.TYPE, ElementType.METHOD})
@Target({ElementType.TYPE, ElementType.METHOD, ElementType.FIELD})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm - right now, the bean scope is defined at either the factory or the bean class itself. I don't know that the dependents ought to decide the scope of the bean its depending on

public @interface RealmScope {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public @interface RealmScope {}
public @interface RealmScope extends AlterableContext {}

And have functionality to perform cleanup when a realm is removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, that's not in the jakarta cdi api jar

Copy link
Member

Choose a reason for hiding this comment

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

jakarta.enterprise.context.spi.AlterableContext

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 @@ -19,6 +19,7 @@
package org.apache.polaris.core.persistence;

import jakarta.annotation.Nonnull;
import jakarta.inject.Inject;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand All @@ -34,7 +35,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;
Expand All @@ -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;

private static final Logger LOGGER =
LoggerFactory.getLogger(LocalPolarisMetaStoreManagerFactory.class);
Expand Down Expand Up @@ -157,12 +157,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;
}
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.RealmScope;
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 */
@RealmScope
Copy link
Member

Choose a reason for hiding this comment

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

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.

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's a fine callout and I think revisiting the EntityCache to be realm-aware, rather than distinct per-realm, is a good change to make, but agreed that it shouldn't be done here

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
Original file line number Diff line number Diff line change
Expand Up @@ -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;

import com.google.api.client.http.javanet.NetHttpTransport;
import com.google.auth.http.HttpTransportFactory;
import com.google.auth.oauth2.GoogleCredentials;
import com.google.cloud.ServiceOptions;
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import jakarta.inject.Named;
import java.util.EnumMap;
import java.util.Map;
import java.util.Set;
import java.util.function.Supplier;
import org.apache.polaris.core.PolarisDiagnostics;
import org.apache.polaris.core.storage.PolarisCredentialProperty;
import org.apache.polaris.core.storage.PolarisStorageActions;
import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo;
import org.apache.polaris.core.storage.PolarisStorageIntegration;
import org.apache.polaris.core.storage.PolarisStorageIntegrationProvider;
import org.apache.polaris.core.storage.aws.AwsCredentialsStorageIntegration;
import org.apache.polaris.core.storage.azure.AzureCredentialsStorageIntegration;
import org.apache.polaris.core.storage.gcp.GcpCredentialsStorageIntegration;
import software.amazon.awssdk.services.sts.StsClient;

@Named("default")
public class PolarisStorageIntegrationProviderImpl implements PolarisStorageIntegrationProvider {

private final Supplier<StsClient> stsClientSupplier;
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
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,7 @@
import org.apache.polaris.core.PolarisConfigurationStore;
import org.apache.polaris.core.auth.PolarisSecretsManager.PrincipalSecretsResult;
import org.apache.polaris.core.persistence.MetaStoreManagerFactory;
import org.apache.polaris.service.config.ConfigurationStoreAware;
import org.apache.polaris.service.config.PolarisApplicationConfig;
import org.apache.polaris.service.context.CallContextResolver;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -48,17 +46,11 @@ protected void run(
Bootstrap<PolarisApplicationConfig> bootstrap,
Namespace namespace,
PolarisApplicationConfig configuration) {
MetaStoreManagerFactory metaStoreManagerFactory = configuration.getMetaStoreManagerFactory();
MetaStoreManagerFactory metaStoreManagerFactory =
Copy link
Member

Choose a reason for hiding this comment

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

Why not @Inject these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dropwizard Commands aren't managed by HK2. We instantiate them directly in PolarisApplication.

Copy link
Member

Choose a reason for hiding this comment

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

But that means that there's a third (CDI, Jackson, manual) way to get those instances. I think, CDI should be used everywhere (except for very specific tests).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

configuration.findService(MetaStoreManagerFactory.class);

PolarisConfigurationStore configurationStore = configuration.getConfigurationStore();
if (metaStoreManagerFactory instanceof ConfigurationStoreAware) {
((ConfigurationStoreAware) metaStoreManagerFactory).setConfigurationStore(configurationStore);
}
CallContextResolver callContextResolver = configuration.getCallContextResolver();
callContextResolver.setMetaStoreManagerFactory(metaStoreManagerFactory);
if (callContextResolver instanceof ConfigurationStoreAware csa) {
csa.setConfigurationStore(configurationStore);
}
PolarisConfigurationStore configurationStore =
configuration.findService(PolarisConfigurationStore.class);

// Execute the bootstrap
Map<String, PrincipalSecretsResult> results =
Expand Down
Loading
Loading