dimas-b commented on code in PR #1139:
URL: https://github.com/apache/polaris/pull/1139#discussion_r1988308045


##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/BaseMetaStoreManager.java:
##########
@@ -45,4 +56,179 @@ public static PolarisStorageConfigurationInfo 
extractStorageConfiguration(
     return PolarisStorageConfigurationInfo.deserialize(
         callCtx.getDiagServices(), storageConfigInfoStr);
   }
+
+  /**
+   * Given the internal property as a map of key/value pairs, serialize it to 
a String
+   *
+   * @param properties a map of key/value pairs
+   * @return a String, the JSON representation of the map
+   */
+  public String serializeProperties(PolarisCallContext callCtx, Map<String, 
String> properties) {
+
+    String jsonString = null;
+    try {
+      // Deserialize the JSON string to a Map<String, String>
+      jsonString = MAPPER.writeValueAsString(properties);
+    } catch (JsonProcessingException ex) {
+      callCtx.getDiagServices().fail("got_json_processing_exception", "ex={}", 
ex);
+    }
+
+    return jsonString;
+  }
+
+  /**
+   * Given the serialized properties, deserialize those to a {@code 
Map<String, String>}
+   *
+   * @param properties a JSON string representing the set of properties
+   * @return a Map of string
+   */
+  public Map<String, String> deserializeProperties(PolarisCallContext callCtx, 
String properties) {
+
+    Map<String, String> retProperties = null;

Review Comment:
   nit: this `null` appears to be returned on parsing exceptions. This looks 
odd to me... Whether `getDiagServices().fail()` throws is very non-intuitive.



##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/AbstractTransactionalPersistence.java:
##########
@@ -194,6 +117,547 @@ protected abstract void deleteFromEntitiesActive(
    * @param callCtx call context
    * @param entity entity record to delete
    */
-  protected abstract void deleteFromEntitiesChangeTracking(
+  protected abstract void deleteFromEntitiesChangeTrackingInCurrentTxn(
       @Nonnull PolarisCallContext callCtx, @Nonnull PolarisEntityCore entity);
+
+  //
+  // Implementations of the one-shot atomic BasePersistence methods which 
explicitly run
+  // the in-transaction variants of methods in a new transaction.
+  //
+
+  /** {@inheritDoc} */
+  @Override
+  public long generateNewId(@Nonnull PolarisCallContext callCtx) {
+    return runInTransaction(callCtx, () -> 
this.generateNewIdInCurrentTxn(callCtx));
+  }
+
+  /** Helper to perform the compare-and-swap semantics of a single writeEntity 
call. */
+  private void checkConditionsForWriteEntityInCurrentTxn(
+      @Nonnull PolarisCallContext callCtx,
+      @Nonnull PolarisBaseEntity entity,
+      @Nullable PolarisBaseEntity originalEntity) {
+    PolarisBaseEntity refreshedEntity =
+        this.lookupEntityInCurrentTxn(
+            callCtx, entity.getCatalogId(), entity.getId(), 
entity.getTypeCode());
+
+    if (originalEntity == null) {
+      if (refreshedEntity != null) {
+        // If this is a "create", and we manage to look up an existing entity 
with already
+        // the same id, where ids are uniquely reserved when generated, it 
means it's a
+        // low-level retry possibly in the face of a transient connectivity 
failure to
+        // the backend database.
+        throw new EntityAlreadyExistsException(refreshedEntity);
+      } else {
+        // Successfully verified the entity doesn't already exist by-id, but 
for a "create"
+        // we must also check for name-collection now.
+        refreshedEntity =
+            this.lookupEntityByNameInCurrentTxn(
+                callCtx,
+                entity.getCatalogId(),
+                entity.getParentId(),
+                entity.getType().getCode(),
+                entity.getName());
+        if (refreshedEntity != null) {
+          // Name-collision conflict.
+          throw new EntityAlreadyExistsException(refreshedEntity);
+        }
+      }
+    } else {
+      // This is an "update".
+      if (refreshedEntity == null
+          || refreshedEntity.getEntityVersion() != 
originalEntity.getEntityVersion()
+          || refreshedEntity.getGrantRecordsVersion() != 
originalEntity.getGrantRecordsVersion()) {

Review Comment:
   I believe this approach implies that the transaction isolation level is 
"serializable", which is fine from my POV, I just want to confirm my 
understanding.



-- 
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: issues-unsubscr...@polaris.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to