dimas-b commented on code in PR #2015:
URL: https://github.com/apache/polaris/pull/2015#discussion_r2195851131
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java:
##########
@@ -82,11 +78,9 @@ public StorageCredentialCache(
/** How long credentials should remain in the cache. */
private long maxCacheDurationMs() {
var cacheDurationSeconds =
- configurationStore.getConfiguration(
- realmContext,
FeatureConfiguration.STORAGE_CREDENTIAL_CACHE_DURATION_SECONDS);
+
realmConfig.getConfig(FeatureConfiguration.STORAGE_CREDENTIAL_CACHE_DURATION_SECONDS);
Review Comment:
I believe it is preferable to invoke `maxCacheDurationMs()` proactively when
a key is inserted and then reuse the value in Caffeine callbacks (line 68).
This duration configuration will remain true to the realm that owns the key.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java:
##########
@@ -82,11 +78,9 @@ public StorageCredentialCache(
/** How long credentials should remain in the cache. */
private long maxCacheDurationMs() {
var cacheDurationSeconds =
- configurationStore.getConfiguration(
- realmContext,
FeatureConfiguration.STORAGE_CREDENTIAL_CACHE_DURATION_SECONDS);
+
realmConfig.getConfig(FeatureConfiguration.STORAGE_CREDENTIAL_CACHE_DURATION_SECONDS);
Review Comment:
I'm not sure this is 100% correct. `RealmConfig` is a request-scoped bean
here, but `maxCacheDurationMs()` may be called by Caffeine for a key in one
ream when the cache is operating in a request context from another realm. This
could cause configuration confusion across realms, I think.
--
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]
