collado-mike commented on code in PR #1356:
URL: https://github.com/apache/polaris/pull/1356#discussion_r2040741421
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java:
##########
@@ -77,7 +78,7 @@ public static CatalogEntity of(PolarisBaseEntity
sourceEntity) {
return null;
}
- public static CatalogEntity fromCatalog(Catalog catalog) {
+ public static CatalogEntity fromCatalog(Catalog catalog, PolarisCallContext
callContext) {
Review Comment:
PolarisCallContext is typically the first argument in the list. I'd maintain
that pattern
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCache.java:
##########
@@ -53,16 +52,22 @@ public class EntityCache {
// index by name
private final AbstractMap<EntityCacheByNameKey, ResolvedPolarisEntity>
byName;
+ private final PolarisCallContext polarisCallContext;
+
/**
* Constructor. Cache can be private or shared
*
* @param polarisMetaStoreManager the meta store manager implementation
*/
- public EntityCache(@Nonnull PolarisMetaStoreManager polarisMetaStoreManager)
{
+ public EntityCache(
+ @Nonnull PolarisMetaStoreManager polarisMetaStoreManager,
+ @Nonnull PolarisCallContext polarisCallContext) {
// by name cache
this.byName = new ConcurrentHashMap<>();
+ this.polarisCallContext = polarisCallContext;
Review Comment:
EntityCache can't have a PolarisCallContext in its constructor and certainly
not as an instance field. The cache lives beyond a single call, so only long
lived contexts should be referenced. Any number of things can change between
calls, even things that might change the configuration store's response.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java:
##########
@@ -178,20 +178,22 @@ public synchronized Supplier<TransactionalPersistence>
getOrCreateSessionSupplie
@Override
public synchronized StorageCredentialCache getOrCreateStorageCredentialCache(
- RealmContext realmContext) {
+ RealmContext realmContext, PolarisCallContext polarisCallContext) {
Review Comment:
You can get from PolarisCallContext to RealmContext, so I don't see a point
in passing in two arguments
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java:
##########
@@ -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) {
Review Comment:
Same comments re: the EntityCache
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/EntityCache.java:
##########
@@ -76,15 +81,22 @@ public EntityCache(@Nonnull PolarisMetaStoreManager
polarisMetaStoreManager) {
};
long weigherTarget =
-
PolarisConfiguration.loadConfig(FeatureConfiguration.ENTITY_CACHE_WEIGHER_TARGET);
+ polarisCallContext
Review Comment:
For cases like this, either the configuration store needs
non-realm-specific config APIs or we should construct an EntityCacheConfig
object at startup that users can set using the application.properties file.
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
