dimas-b commented on code in PR #4397:
URL: https://github.com/apache/polaris/pull/4397#discussion_r3235267855
##########
persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStoreManagerFactory.java:
##########
@@ -132,7 +135,26 @@ public EntityCache getOrCreateEntityCache(RealmContext
realmContext, RealmConfig
}
@Override
- public BasePersistence getOrCreateSession(RealmContext realmContext) {
+ public BasePersistence getOrCreateBasePersistence(RealmContext realmContext)
{
+ return newNoSqlMetaStore(realmContext);
+ }
+
+ @Override
+ public PolicyMappingPersistence
getOrCreatePolicyMappingPersistence(RealmContext realmContext) {
+ return newNoSqlMetaStore(realmContext);
+ }
+
+ @Override
+ public MetricsPersistence getOrCreateMetricsPersistence(RealmContext
realmContext) {
+ return newNoSqlMetaStore(realmContext);
Review Comment:
I think, this should return a no-op `new MetricsPersistence() {}` for the
sake of clarity - NoSQL persistence does not implement it anyway.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##########
@@ -26,13 +26,25 @@
import org.apache.polaris.core.persistence.cache.EntityCache;
import org.apache.polaris.core.persistence.dao.entity.BaseResult;
import org.apache.polaris.core.persistence.dao.entity.PrincipalSecretsResult;
+import org.apache.polaris.core.persistence.metrics.MetricsPersistence;
+import org.apache.polaris.core.policy.PolicyMappingPersistence;
/** Configuration interface for configuring the {@link
PolarisMetaStoreManager}. */
public interface MetaStoreManagerFactory {
PolarisMetaStoreManager getOrCreateMetaStoreManager(RealmContext
realmContext);
- BasePersistence getOrCreateSession(RealmContext realmContext);
+ /** Returns the per-realm {@link BasePersistence}. */
+ BasePersistence getOrCreateBasePersistence(RealmContext realmContext);
+
+ /** Returns the per-realm {@link PolicyMappingPersistence}. */
+ PolicyMappingPersistence getOrCreatePolicyMappingPersistence(RealmContext
realmContext);
+
+ /** Returns the per-realm {@link MetricsPersistence}. */
+ MetricsPersistence getOrCreateMetricsPersistence(RealmContext realmContext);
Review Comment:
`MetricsPersistence` is very different from "meta store" persistence... both
functionally and in java SPI parameters. I believe it makes sense to use a
separate factory for it.
That said, current changes are net positive, IMHO, and are moving in the
right direction.
I think we can deal with the `MetricsPersistence` factory in a separate PR.
##########
persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStoreManagerFactory.java:
##########
@@ -132,7 +132,7 @@ public EntityCache getOrCreateEntityCache(RealmContext
realmContext, RealmConfig
}
@Override
- public BasePersistence getOrCreateSession(RealmContext realmContext) {
+ public BasePersistence getOrCreateBasePersistence(RealmContext realmContext)
{
Review Comment:
However... I personally already reviewed those rename changes :sweat_smile:
so from my POV they are ok to keep in this PR.
--
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]
