HDDS-11408. Address review comments

Change-Id: I014300b4597d4c9c022c3074ab612885816696fe
apache · Sep 18, 2024 · e2f0461 · e2f0461
1 parent 4b724fe
commit e2f0461
Show file tree

Hide file tree

Showing 2 changed files with 14 additions and 10 deletions.
diff --git a/...one-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java b/...one-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java
@@ -122,6 +122,8 @@ public SnapshotDeletingService(long interval, long serviceTimeout,
     this.serviceTimeout = serviceTimeout;
   }
 
+  // Wait for a notification from KeyDeletingService if the key deletion is running. This is to ensure, merging of
+  // entries do not start while the AOS is still processing the deleted keys.
   @VisibleForTesting
   public void waitForKeyDeletingService() throws InterruptedException {
     KeyDeletingService keyDeletingService = getOzoneManager().getKeyManager().getDeletingService();
@@ -132,6 +134,8 @@ public void waitForKeyDeletingService() throws InterruptedException {
     }
   }
 
+  // Wait for a notification from DirectoryDeletingService if the directory deletion is running. This is to ensure,
+  // merging of entries do not start while the AOS is still processing the deleted keys.
   @VisibleForTesting
   public void waitForDirDeletingService() throws InterruptedException {
     DirectoryDeletingService directoryDeletingService = getOzoneManager().getKeyManager()
@@ -157,7 +161,7 @@ public BackgroundTaskResult call() throws InterruptedException {
       try {
         int remaining = keyLimitPerTask;
         Iterator<UUID> iterator = snapshotChainManager.iterator(true);
-        List<SnapshotInfo> snapshotsToBePurged = new ArrayList<>();
+        List<String> snapshotsToBePurged = new ArrayList<>();
         long snapshotLimit = snapshotDeletionPerTask;
         while (iterator.hasNext() && snapshotLimit > 0) {
           SnapshotInfo snapInfo = SnapshotUtils.getSnapshotInfo(ozoneManager, snapshotChainManager, iterator.next());
@@ -228,15 +232,14 @@ public BackgroundTaskResult call() throws InterruptedException {
                 throw e.getCause();
               }
             } else {
-              snapshotsToBePurged.add(snapInfo);
+              snapshotsToBePurged.add(snapInfo.getTableKey());
             }
           }
           successRunCount.incrementAndGet();
           snapshotLimit--;
         }
         if (!snapshotsToBePurged.isEmpty()) {
-          submitSnapshotPurgeRequest(snapshotsToBePurged.stream().map(SnapshotInfo::getTableKey)
-              .collect(Collectors.toList()));
+          submitSnapshotPurgeRequest(snapshotsToBePurged);
         }
       } catch (IOException e) {
         LOG.error("Error while running Snapshot Deleting Service", e);
@@ -303,7 +306,7 @@ private void submitRequest(OMRequest omRequest) {
       try {
         OzoneManagerRatisUtils.submitRequest(ozoneManager, omRequest, clientId, getRunCount().get());
       } catch (ServiceException e) {
-        LOG.error("Snapshot Deleting request failed. Will retry at next run.", e);
+        LOG.error("Request: {} fired by SnapshotDeletingService failed. Will retry in the next run", omRequest, e);
       }
     }
   }

diff --git a/...-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotUtils.java b/...-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotUtils.java
@@ -244,7 +244,7 @@ public static String getOzonePathKeyForFso(OMMetadataManager metadataManager,
   }
 
   /**
-   * Creates merged repeatedKeyInfo entry with the existing deleted entry in the table.
+   * Returns merged repeatedKeyInfo entry with the existing deleted entry in the table.
    * @param snapshotMoveKeyInfos keyInfos to be added.
    * @param metadataManager metadataManager for a store.
    * @return
@@ -261,9 +261,10 @@ public static RepeatedOmKeyInfo createMergedRepeatedOmKeyInfoFromDeletedTableEnt
     }
     // When older version of keys are moved to the next snapshot's deletedTable
     // The newer version might also be in the next snapshot's deletedTable and
-    // it might overwrite. This is to avoid that and also avoid having
-    // orphans blocks. Checking the last keyInfoList size omKeyInfo versions,
-    // this is to avoid redundant additions if the last n versions match.
+    // it might overwrite the existing value which inturn could lead to orphan block in the system.
+    // Checking the keyInfoList with the last n versions of the omKeyInfo versions would ensure all versions are
+    // present in the list and would also avoid redundant additions to the list if the last n versions match, which
+    // can happen on om transaction replay on snapshotted rocksdb.
     RepeatedOmKeyInfo result = metadataManager.getDeletedTable().get(dbKey);
     if (result == null) {
       result = new RepeatedOmKeyInfo(keyInfoList);
@@ -274,7 +275,7 @@ public static RepeatedOmKeyInfo createMergedRepeatedOmKeyInfoFromDeletedTableEnt
   }
 
   private static boolean isSameAsLatestOmKeyInfo(List<OmKeyInfo> omKeyInfos,
-                                                RepeatedOmKeyInfo result) {
+                                                 RepeatedOmKeyInfo result) {
     int size = result.getOmKeyInfoList().size();
     if (size >= omKeyInfos.size()) {
       return omKeyInfos.equals(result.getOmKeyInfoList().subList(size - omKeyInfos.size(), size));