dimas-b commented on code in PR #4397:
URL: https://github.com/apache/polaris/pull/4397#discussion_r3230307476
##########
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:
nit: this rename causes insignificant changes in the context of this PR,
which complicates review. I would be nicer to do the renaming before or after
this PR 😅
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/MetaStoreManagerFactory.java:
##########
@@ -26,13 +26,59 @@
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}. Backends implement this on
the same concrete
+ * class as the other modular persistences (such as {@link
PolicyMappingPersistence}, {@link
+ * MetricsPersistence}, and {@link IntegrationPersistence}); the
corresponding typed accessors
+ * below default to casting that same instance.
+ */
+ BasePersistence getOrCreateBasePersistence(RealmContext realmContext);
+
+ /**
+ * Returns the per-realm {@link PolicyMappingPersistence}. The default
implementation returns the
+ * same instance produced by {@link
#getOrCreateBasePersistence(RealmContext)} when it also
+ * implements {@link PolicyMappingPersistence}.
+ */
+ default PolicyMappingPersistence
getOrCreatePolicyMappingPersistence(RealmContext realmContext) {
+ return cast(getOrCreateBasePersistence(realmContext),
PolicyMappingPersistence.class);
Review Comment:
TBH, I do not really understand the purpose behind this design. We're still
forcing implementation classes to support all the same interfaces that used to
be extended by `BasePersistence`. IMHO, this make the overall design more
awkward than before.
I believe the idea is/was to inject `PolicyMappingPersistence` into callers
independently of `BasePersistence`. If they happen to be implemented by the
same class, it should just work. However, we should not assume that (and use
casts).
--
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]
