dimas-b commented on code in PR #4397:
URL: https://github.com/apache/polaris/pull/4397#discussion_r3235326569
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -129,6 +132,19 @@ private BasePersistence createSession(
schemaVersion);
}
+ private JdbcBasePersistenceImpl getOrCreateJdbcPersistence(RealmContext
realmContext) {
Review Comment:
there's no "get" logic in this method, it always "creates". Please consider
renaming to `createJdbcPersistence()` for clarity.
##########
persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NonFunctionalBasePersistence.java:
##########
@@ -35,17 +35,23 @@
import org.apache.polaris.core.entity.PolarisPrincipalSecrets;
import org.apache.polaris.core.persistence.BasePersistence;
import org.apache.polaris.core.persistence.IntegrationPersistence;
+import org.apache.polaris.core.persistence.metrics.MetricsPersistence;
import org.apache.polaris.core.persistence.pagination.Page;
import org.apache.polaris.core.persistence.pagination.PageToken;
import org.apache.polaris.core.policy.PolarisPolicyMappingRecord;
+import org.apache.polaris.core.policy.PolicyMappingPersistence;
import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo;
import org.apache.polaris.core.storage.PolarisStorageIntegration;
import org.jspecify.annotations.NonNull;
import org.jspecify.annotations.Nullable;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-abstract class NonFunctionalBasePersistence implements BasePersistence,
IntegrationPersistence {
+abstract class NonFunctionalBasePersistence
+ implements BasePersistence,
+ IntegrationPersistence,
+ MetricsPersistence,
Review Comment:
Please remove `MetricsPersistence` from this list - it's not really
implemented.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java:
##########
@@ -229,8 +231,10 @@ private void dropEntity(
final List<PolarisPolicyMappingRecord> mappingOnTarget =
(entity.getType() == PolarisEntityType.POLICY)
? List.of()
- : ms.loadAllPoliciesOnTarget(callCtx, entity.getCatalogId(),
entity.getId());
- ms.deleteAllEntityPolicyMappingRecords(callCtx, entity,
mappingOnTarget, mappingOnPolicy);
+ : policyMappingPersistence.loadAllPoliciesOnTarget(
+ callCtx, entity.getCatalogId(), entity.getId());
+ policyMappingPersistence.deleteAllEntityPolicyMappingRecords(
Review Comment:
I think this goes against the grain of `AtomicOperationMetaStoreManager`
javadoc, which says the class operates on _one_ `BasePersistence` instance
(logically).
IMHO, even the old code is not "atomic" with respect to
`ms.deleteAllEntityGrantRecords` (line 213) and
`deleteAllEntityPolicyMappingRecords` (line 236), but let's not aggravate this
situation by introducing different objects. The old code at least invoked both
methods on the same java object instance.
Sorry for missing this aspect when I initially suggested breaking
`BasePersistece` down into all individual sub-interfaces. This code is not
super easy to comprehend and keep in mind all the time 😅
With this code in mind, I think it makes sense to isolate only
`MetricsPersistence` in this PR (and isolate idempotency persistence in #4269).
I think we need to keep this general difficulty (atomic operations) in mind
for future `BasePersistece` refactorings, but for now I think it makes sense to
keep "old" functionality intact and only isolate "new" functionality into
purpose-based interfaces (`MetricsPersistence`, `IdempotencyPersistence`, etc.)
--
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]
