-
Notifications
You must be signed in to change notification settings - Fork 369
Remove PolarisConfiguration.loadConfig #1356
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
Changes from all commits
6f7eb2f
50bd7b1
c11d40a
df1fa0c
20dddc3
6e20597
519c436
5d5783a
f3f081f
20326d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -178,21 +178,23 @@ public synchronized Supplier<TransactionalPersistence> getOrCreateSessionSupplie | |
|
|
||
| @Override | ||
| public synchronized StorageCredentialCache getOrCreateStorageCredentialCache( | ||
| RealmContext realmContext) { | ||
| RealmContext realmContext, PolarisCallContext polarisCallContext) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can get from PolarisCallContext to RealmContext, so I don't see a point in passing in two arguments
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is that true? I think you can get from CallContext to RealmContext, but not from PolarisCallContext. Personally I would like to unify these in the future but I want to keep this PR tightly scoped. |
||
| if (!storageCredentialCacheMap.containsKey(realmContext.getRealmIdentifier())) { | ||
| storageCredentialCacheMap.put( | ||
| realmContext.getRealmIdentifier(), new StorageCredentialCache()); | ||
| realmContext.getRealmIdentifier(), new StorageCredentialCache(polarisCallContext)); | ||
| } | ||
|
|
||
| return storageCredentialCacheMap.get(realmContext.getRealmIdentifier()); | ||
| } | ||
|
|
||
| @Override | ||
| public synchronized EntityCache getOrCreateEntityCache(RealmContext realmContext) { | ||
| public synchronized EntityCache getOrCreateEntityCache( | ||
| RealmContext realmContext, PolarisCallContext polarisCallContext) { | ||
| if (!entityCacheMap.containsKey(realmContext.getRealmIdentifier())) { | ||
| PolarisMetaStoreManager metaStoreManager = getOrCreateMetaStoreManager(realmContext); | ||
| entityCacheMap.put( | ||
| realmContext.getRealmIdentifier(), new InMemoryEntityCache(metaStoreManager)); | ||
| realmContext.getRealmIdentifier(), | ||
| new InMemoryEntityCache(metaStoreManager, polarisCallContext)); | ||
| } | ||
|
|
||
| return entityCacheMap.get(realmContext.getRealmIdentifier()); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |
|
|
||
| import java.util.Map; | ||
| import java.util.function.Supplier; | ||
| import org.apache.polaris.core.PolarisCallContext; | ||
| import org.apache.polaris.core.context.RealmContext; | ||
| import org.apache.polaris.core.persistence.bootstrap.RootCredentialsSet; | ||
| import org.apache.polaris.core.persistence.cache.EntityCache; | ||
|
|
@@ -34,9 +35,11 @@ public interface MetaStoreManagerFactory { | |
|
|
||
| Supplier<? extends BasePersistence> getOrCreateSessionSupplier(RealmContext realmContext); | ||
|
|
||
| StorageCredentialCache getOrCreateStorageCredentialCache(RealmContext realmContext); | ||
| StorageCredentialCache getOrCreateStorageCredentialCache( | ||
| RealmContext realmContext, PolarisCallContext polarisCallContext); | ||
|
|
||
| EntityCache getOrCreateEntityCache(RealmContext realmContext); | ||
| EntityCache getOrCreateEntityCache( | ||
| RealmContext realmContext, PolarisCallContext polarisCallContext); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not support passing context parameter as method args in general and this PR adds another context parameter. Could we leverage CDI for injecting context data?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good callout. In general I agree that we can leverage CDI to inject context parameters (e.g. PolarisCallContext) around. However in this particular case, the EntityCache is not a CDI-managed class. There's a discussion above about this too, but I think we need a way to have configs that are not tied to a PolarisCallContext('s configuration store). |
||
|
|
||
| Map<String, PrincipalSecretsResult> bootstrapRealms( | ||
| Iterable<String> realms, RootCredentialsSet rootCredentialsSet); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,6 @@ | |
| import org.apache.polaris.core.PolarisCallContext; | ||
| import org.apache.polaris.core.config.BehaviorChangeConfiguration; | ||
| import org.apache.polaris.core.config.FeatureConfiguration; | ||
| import org.apache.polaris.core.config.PolarisConfiguration; | ||
| import org.apache.polaris.core.entity.PolarisBaseEntity; | ||
| import org.apache.polaris.core.entity.PolarisEntityType; | ||
| import org.apache.polaris.core.entity.PolarisGrantRecord; | ||
|
|
@@ -58,7 +57,9 @@ public class InMemoryEntityCache implements EntityCache { | |
| * | ||
| * @param polarisMetaStoreManager the meta store manager implementation | ||
| */ | ||
| public InMemoryEntityCache(@Nonnull PolarisMetaStoreManager polarisMetaStoreManager) { | ||
| public InMemoryEntityCache( | ||
| @Nonnull PolarisMetaStoreManager polarisMetaStoreManager, | ||
| @Nonnull PolarisCallContext polarisCallContext) { | ||
|
|
||
| // by name cache | ||
| this.byName = new ConcurrentHashMap<>(); | ||
|
|
@@ -76,15 +77,22 @@ public InMemoryEntityCache(@Nonnull PolarisMetaStoreManager polarisMetaStoreMana | |
| }; | ||
|
|
||
| long weigherTarget = | ||
| PolarisConfiguration.loadConfig(FeatureConfiguration.ENTITY_CACHE_WEIGHER_TARGET); | ||
| polarisCallContext | ||
| .getConfigurationStore() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked through the code and as far as I can see all instances of If injection through CDI seems too intrusive for this PR, could we instead use
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but TBH, I'd really like to use CDI (with custom producers) for injection rather than relying on factories in the main call path. In other words, I'd like the main body of code to receive dependent objects through injection and factories / producers to be removed from the main call paths and be engaged by CDI when request-handlers are constructed. All-in-all, the intention behind this PR seems to be pointing in that direction, so why not do the CDI-based refactoring now and then perhaps the config loading issue is solved automatically? |
||
| .getConfiguration(polarisCallContext, FeatureConfiguration.ENTITY_CACHE_WEIGHER_TARGET); | ||
| Caffeine<Long, ResolvedPolarisEntity> byIdBuilder = | ||
| Caffeine.newBuilder() | ||
| .maximumWeight(weigherTarget) | ||
| .weigher(EntityWeigher.asWeigher()) | ||
| .expireAfterAccess(1, TimeUnit.HOURS) // Expire entries after 1 hour of no access | ||
| .removalListener(removalListener); // Set the removal listener | ||
|
|
||
| if (PolarisConfiguration.loadConfig(BehaviorChangeConfiguration.ENTITY_CACHE_SOFT_VALUES)) { | ||
| boolean useSoftValues = | ||
| polarisCallContext | ||
| .getConfigurationStore() | ||
| .getConfiguration( | ||
| polarisCallContext, BehaviorChangeConfiguration.ENTITY_CACHE_SOFT_VALUES); | ||
| if (useSoftValues) { | ||
| byIdBuilder.softValues(); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,7 +31,6 @@ | |
| import org.apache.iceberg.exceptions.UnprocessableEntityException; | ||
| import org.apache.polaris.core.PolarisCallContext; | ||
| import org.apache.polaris.core.config.FeatureConfiguration; | ||
| import org.apache.polaris.core.config.PolarisConfiguration; | ||
| import org.apache.polaris.core.entity.PolarisEntity; | ||
| import org.apache.polaris.core.entity.PolarisEntityType; | ||
| import org.apache.polaris.core.persistence.dao.entity.ScopedCredentialsResult; | ||
|
|
@@ -47,8 +46,11 @@ public class StorageCredentialCache { | |
| private static final long CACHE_MAX_NUMBER_OF_ENTRIES = 10_000L; | ||
| private final LoadingCache<StorageCredentialCacheKey, StorageCredentialCacheEntry> cache; | ||
|
|
||
| private final long maxCacheDurationMs; | ||
|
|
||
| /** Initialize the creds cache */ | ||
| public StorageCredentialCache() { | ||
| public StorageCredentialCache(PolarisCallContext polarisCallContext) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comments re: the EntityCache |
||
| this.maxCacheDurationMs = maxCacheDurationMs(polarisCallContext); | ||
| cache = | ||
| Caffeine.newBuilder() | ||
| .maximumSize(CACHE_MAX_NUMBER_OF_ENTRIES) | ||
|
|
@@ -60,7 +62,7 @@ public StorageCredentialCache() { | |
| 0, | ||
| Math.min( | ||
| (entry.getExpirationTime() - System.currentTimeMillis()) / 2, | ||
| maxCacheDurationMs())); | ||
| this.maxCacheDurationMs)); | ||
| return Duration.ofMillis(expireAfterMillis); | ||
| })) | ||
| .build( | ||
|
|
@@ -71,12 +73,17 @@ public StorageCredentialCache() { | |
| } | ||
|
|
||
| /** How long credentials should remain in the cache. */ | ||
| private static long maxCacheDurationMs() { | ||
| var cacheDurationSeconds = | ||
| PolarisConfiguration.loadConfig( | ||
| FeatureConfiguration.STORAGE_CREDENTIAL_CACHE_DURATION_SECONDS); | ||
| var credentialDurationSeconds = | ||
| PolarisConfiguration.loadConfig(FeatureConfiguration.STORAGE_CREDENTIAL_DURATION_SECONDS); | ||
| private static long maxCacheDurationMs(PolarisCallContext polarisCallContext) { | ||
| int cacheDurationSeconds = | ||
| polarisCallContext | ||
| .getConfigurationStore() | ||
| .getConfiguration( | ||
| polarisCallContext, FeatureConfiguration.STORAGE_CREDENTIAL_CACHE_DURATION_SECONDS); | ||
| int credentialDurationSeconds = | ||
| polarisCallContext | ||
| .getConfigurationStore() | ||
| .getConfiguration( | ||
| polarisCallContext, FeatureConfiguration.STORAGE_CREDENTIAL_DURATION_SECONDS); | ||
| if (cacheDurationSeconds >= credentialDurationSeconds) { | ||
| throw new IllegalArgumentException( | ||
| String.format( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This look strange to me. We cache
StorageCredentialCacheonly by realm ID, but the instance references a potentially oldPolarisCallContext. Can we be sure thatPolarisCallContextinside theStorageCredentialCacheis still relevant when it is obtained from the map later?Currently we only use
polarisCallContextfor resolving config parameters, still, it is very non-intuitive that the config values resolved in onepolarisCallContextare applicable to anotherpolarisCallContexteven within the same realm 🤔