-
Notifications
You must be signed in to change notification settings - Fork 271
Use application-scoped StorageCredentialCache #2022
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
base: main
Are you sure you want to change the base?
Use application-scoped StorageCredentialCache #2022
Conversation
Converted to "draft state", as it depends on another PR |
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.
Thanks a ton for this!
...src/test/java/org/apache/polaris/service/quarkus/catalog/PolarisGenericTableCatalogTest.java
Outdated
Show resolved
Hide resolved
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.
Nice refactoring for application-scoped data. From my POV, this PR improves control over Polaris heap usage by consolidating cached credentials in one cache object as opposed to having a (virtually unbounded) collection of caches.
public StorageCredentialCache( | ||
RealmContext realmContext, PolarisConfigurationStore configurationStore) { | ||
this.realmContext = realmContext; | ||
public StorageCredentialCache(PolarisConfigurationStore configurationStore) { | ||
this.configurationStore = configurationStore; | ||
cache = | ||
Caffeine.newBuilder() |
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.
Would you mind making the max cache size (by entries) configurable? Now that the caches is shared across realms, configuring its side is more important to users, I guess.
I suppose the cache size has to be a global configuration (maybe a constructor parameter injected via CDI producers + Quarkus config).
polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheKey.java
Show resolved
Hide resolved
eb13f96
to
7f1eaf4
Compare
7f1eaf4
to
bf5a42e
Compare
rebased after 19f44d8 due to conflicts |
currently based on #2021
since
StorageCredentialCache
is application scoped and its constructor no longer uses theRealmContext
passed intogetOrCreateStorageCredentialCache
we can now let allPolarisEntityManager
instances share the sameStorageCredentialCache
.