XN137 commented on code in PR #2229:
URL: https://github.com/apache/polaris/pull/2229#discussion_r2251198171
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/BaseMetaStoreManager.java:
##########
@@ -53,6 +56,16 @@ public static PolarisStorageConfigurationInfo
extractStorageConfiguration(
return PolarisStorageConfigurationInfo.deserialize(storageConfigInfoStr);
}
+ private final Supplier<MS> metaStoreSupplier;
+
+ public BaseMetaStoreManager(Supplier<MS> metaStoreSupplier) {
+ this.metaStoreSupplier = Suppliers.memoize(metaStoreSupplier::get);
Review Comment:
it is still unclear what the actual lifetime scope of the `BasePersistence`
should be here.
on main its lifetime is bound to the `PolarisCallContext`.
when we use `memoize` here, we'd have a single instance per realm.
when we dont use `memoize` here, we would be creating more instances than
before, because for example
`AtomicOperationMetaStoreManager#loadResolvedEntityByName` calls
`AtomicOperationMetaStoreManager#createEntityIfNotExists` internally, both of
which call `getMetaStore()`.
i am wondering if the subclasses of BaseMetaStoreManager would need to tell
the base class how to use the Supplier, depending on whether their specific
BasePersistence impl instance has state or not.
theoretically we could also let BaseMetaStoreManager have a cache keyed by
PolarisCallContext to keep exactly the existing "instance per callcontext"
semantics though it would look a bit hacky
--
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]
