dimas-b commented on code in PR #1356:
URL: https://github.com/apache/polaris/pull/1356#discussion_r2070905604
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -197,21 +197,23 @@ public synchronized Supplier<BasePersistence>
getOrCreateSessionSupplier(
@Override
public synchronized StorageCredentialCache getOrCreateStorageCredentialCache(
- RealmContext realmContext) {
+ RealmContext realmContext, PolarisCallContext polarisCallContext) {
if
(!storageCredentialCacheMap.containsKey(realmContext.getRealmIdentifier())) {
storageCredentialCacheMap.put(
- realmContext.getRealmIdentifier(), new StorageCredentialCache());
+ realmContext.getRealmIdentifier(), new
StorageCredentialCache(polarisCallContext));
Review Comment:
This look strange to me. We cache `StorageCredentialCache` only by realm ID,
but the instance references a potentially old `PolarisCallContext`. Can we be
sure that `PolarisCallContext` inside the `StorageCredentialCache` is still
relevant when it is obtained from the map later?
Currently we only use `polarisCallContext` for resolving config parameters,
still, it is very non-intuitive that the config values resolved in one
`polarisCallContext` are applicable to another `polarisCallContext` even within
the same realm :thinking:
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/cache/InMemoryEntityCache.java:
##########
@@ -76,15 +77,22 @@ public InMemoryEntityCache(@Nonnull PolarisMetaStoreManager
polarisMetaStoreMana
};
long weigherTarget =
-
PolarisConfiguration.loadConfig(FeatureConfiguration.ENTITY_CACHE_WEIGHER_TARGET);
+ polarisCallContext
+ .getConfigurationStore()
Review Comment:
I looked through the code and as far as I can see all instances of
`PolarisConfigurationStore` happen to be application-scoped. Could we avoid
requiring a "call" (i.e. request) context for getting
`PolarisConfigurationStore`?
If injection through CDI seems too intrusive for this PR, could we instead
use `PolarisConfigurationStore` as the new parameter (instead of
`PolarisCallContext`)?
--
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]
