sumitagrawl commented on code in PR #4811:
URL: https://github.com/apache/ozone/pull/4811#discussion_r1229899139


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1543,17 +1535,14 @@ public List<BlockGroup> getPendingDeletionKeys(final 
int keyCount,
               // have to check deletedTable of previous snapshot
               RepeatedOmKeyInfo delOmKeyInfo =
                   prevDeletedTable.get(prevDelTableDBKey);
-              if ((omKeyInfo != null &&
-                  info.getObjectID() == omKeyInfo.getObjectID()) ||
-                  delOmKeyInfo != null) {
-                // TODO: [SNAPSHOT] For now, we are not cleaning up a key in
-                //  active DB's deletedTable if any one of the keys in
-                //  RepeatedOmKeyInfo exists in last snapshot's key/fileTable.
-                //  Might need to refactor OMKeyDeleteRequest first to take
-                //  actual reclaimed key objectIDs as input
-                //  in order to avoid any race condition.
-                blockGroupList.clear();
-                break;
+              if (versionExistsInPreviousSnapshot(omKeyInfo,
+                  info, delOmKeyInfo)) {
+                // If the infoList size is 1, there is nothing to split.
+                // We either delete it or skip it.
+                if (!(infoList.getOmKeyInfoList().size() == 1)) {

Review Comment:
   If having OmKeyInfoList() size is > "1", its neither added to 
notReclaimableKeyInfo nor to blockGroupList, so how cleanup will happen?



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -530,11 +532,14 @@ private boolean isKeyReclaimable(
       OmKeyInfo prevKeyInfo = renamedKey != null ? previousKeyTable
           .get(renamedKey) : previousKeyTable.get(dbKey);
 
-      if (prevKeyInfo == null) {
+      if (prevKeyInfo == null ||
+          prevKeyInfo.getObjectID() != deletedKeyInfo.getObjectID()) {
         return true;
       }
 
-      return prevKeyInfo.getObjectID() != deletedKeyInfo.getObjectID();
+      // For key overwrite the objectID will remain the same, In this

Review Comment:
   For Hsync, blockLocations may not be same, but still same object, may need 
handle this scenario



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyPurgeResponse.java:
##########
@@ -69,6 +77,22 @@ public void addToDBBatch(OMMetadataManager omMetadataManager,
       }
     } else {
       processKeys(batchOperation, omMetadataManager);
+      processKeysToUpdate(batchOperation, omMetadataManager);
+    }
+  }
+
+  private void processKeysToUpdate(BatchOperation batchOp,
+      OMMetadataManager metadataManager) throws IOException {
+    if (keysToUpdateList == null) {
+      return;
+    }
+
+    for (SnapshotMoveKeyInfos keyToUpdate : keysToUpdateList) {
+      List<KeyInfo> keyInfosList = keyToUpdate.getKeyInfosList();
+      RepeatedOmKeyInfo repeatedOmKeyInfo =
+          createRepeatedOmKeyInfo(keyInfosList);
+      metadataManager.getDeletedTable().putWithBatch(batchOp,

Review Comment:
   If this is put back to same deletedTable, keyDeletingService keeps on 
retrying ... do the above case of all version re-claimable -- not added to any 
list used to break this retry ?



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1571,11 +1560,33 @@ public List<BlockGroup> getPendingDeletionKeys(final 
int keyCount,
             }
             currentCount++;
           }
-          keyBlocksList.addAll(blockGroupList);
+
+          List<OmKeyInfo> notReclaimableKeyInfoList =
+              notReclaimableKeyInfo.getOmKeyInfoList();
+
+          // If all the versions are not reclaimable, then do nothing.
+          if (notReclaimableKeyInfoList.size() > 0 &&

Review Comment:
   3 case, not getting if all scenario handled:
   1. all versions are not reclaimable - handled
   2. all versions are reclaimable - not getting how handled
   3. blocks are added if few blocks are reclaimable



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1571,11 +1560,33 @@ public List<BlockGroup> getPendingDeletionKeys(final 
int keyCount,
             }
             currentCount++;
           }
-          keyBlocksList.addAll(blockGroupList);
+
+          List<OmKeyInfo> notReclaimableKeyInfoList =
+              notReclaimableKeyInfo.getOmKeyInfoList();
+
+          // If all the versions are not reclaimable, then do nothing.
+          if (notReclaimableKeyInfoList.size() > 0 &&
+              notReclaimableKeyInfoList.size() !=
+                  infoList.getOmKeyInfoList().size()) {
+            keysToModify.put(kv.getKey(), notReclaimableKeyInfo);
+          }
+
+          if (notReclaimableKeyInfoList.size() !=
+              infoList.getOmKeyInfoList().size()) {
+            keyBlocksList.addAll(blockGroupList);
+          }
         }
       }
     }
-    return keyBlocksList;
+    return new PendingKeysDeletion(keyBlocksList, keysToModify);
+  }
+
+  private boolean versionExistsInPreviousSnapshot(OmKeyInfo omKeyInfo,
+      OmKeyInfo info, RepeatedOmKeyInfo delOmKeyInfo) {
+    return (omKeyInfo != null &&

Review Comment:
   Need handle for HSync, where object will be same but blocks are different as 
append mechanism



-- 
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]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to