HDDS-8064. [Snapshot] Clean up FSO deletedDirTable as well during snapshot creation

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java

@@ -18,6 +18,7 @@
 
 package org.apache.hadoop.ozone.om;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.cache.CacheBuilder;
 import com.google.common.cache.CacheLoader;
@@ -53,6 +54,7 @@
 import org.apache.hadoop.hdds.utils.db.managed.ManagedRocksDB;
 import org.apache.hadoop.ozone.om.codec.OmDBDiffReportEntryCodec;
 import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
 import org.apache.hadoop.ozone.om.helpers.RepeatedOmKeyInfo;
 import org.apache.hadoop.ozone.om.helpers.SnapshotInfo;
 import org.apache.hadoop.ozone.om.service.SnapshotDiffCleanupService;
@@ -372,15 +374,19 @@ public static DBCheckpoint createOmSnapshotCheckpoint(
     // Acquire deletedTable write lock
     omMetadataManager.getTableLock(OmMetadataManagerImpl.DELETED_TABLE)
         .writeLock().lock();
+    // TODO: [SNAPSHOT] HDDS-8067. Acquire deletedDirectoryTable write lock
     try {
       // Create DB checkpoint for snapshot
       dbCheckpoint = store.getSnapshot(snapshotInfo.getCheckpointDirName());
       // Clean up active DB's deletedTable right after checkpoint is taken,
       // with table write lock held
-      deleteKeysInSnapshotScopeFromDTableInternal(omMetadataManager,
+      deleteKeysFromDelKeyTableInSnapshotScope(omMetadataManager,
+          snapshotInfo.getVolumeName(), snapshotInfo.getBucketName());
+      // Clean up deletedDirectoryTable as well
+      deleteKeysFromDelDirTableInSnapshotScope(omMetadataManager,
           snapshotInfo.getVolumeName(), snapshotInfo.getBucketName());
-      // TODO: [SNAPSHOT] HDDS-8064. Clean up deletedDirTable as well
     } finally {
+      // TODO: [SNAPSHOT] HDDS-8067. Release deletedDirectoryTable write lock
       // Release deletedTable write lock
       omMetadataManager.getTableLock(OmMetadataManagerImpl.DELETED_TABLE)
           .writeLock().unlock();
@@ -413,73 +419,153 @@ public static DBCheckpoint createOmSnapshotCheckpoint(
   }
 
   /**
-   * Helper method to delete keys in the snapshot scope from active DB's
-   * deletedTable.
-   *
+   * Helper method to delete DB keys in the snapshot scope (bucket)
+   * from active DB's deletedDirectoryTable.
    * @param omMetadataManager OMMetadataManager instance
    * @param volumeName volume name
    * @param bucketName bucket name
    */
-  private static void deleteKeysInSnapshotScopeFromDTableInternal(
+  private static void deleteKeysFromDelDirTableInSnapshotScope(
       OMMetadataManager omMetadataManager,
       String volumeName,
       String bucketName) throws IOException {
 
     // Range delete start key (inclusive)
-    String beginKey =
-        omMetadataManager.getOzoneKey(volumeName, bucketName, "");
-
-    // Range delete end key (exclusive) to be found
+    final String beginKey = getOzonePathKeyWithVolumeBucketNames(
+        omMetadataManager, volumeName, bucketName);
+    // Range delete end key (exclusive). To be calculated
     String endKey;
 
-    // Start performance tracking timer
-    long startTime = System.nanoTime();
+    try (TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
+         iter = omMetadataManager.getDeletedDirTable().iterator()) {
+      endKey = findEndKeyGivenPrefix(iter, beginKey);
+    }
 
-    try (TableIterator<String,
-        ? extends Table.KeyValue<String, RepeatedOmKeyInfo>>
-             keyIter = omMetadataManager.getDeletedTable().iterator()) {
-
-      keyIter.seek(beginKey);
-      // Continue only when there are entries of snapshot (bucket) scope
-      // in deletedTable in the first place
-      if (!keyIter.hasNext()) {
-        // Use null as a marker. No need to do deleteRange() at all.
-        endKey = null;
-      } else {
-        // Remember the last key with a matching prefix
-        endKey = keyIter.next().getKey();
-
-        // Loop until prefix mismatches.
-        // TODO: [SNAPSHOT] Try to seek next predicted bucket name (speed up?)
-        while (keyIter.hasNext()) {
-          Table.KeyValue<String, RepeatedOmKeyInfo> entry = keyIter.next();
-          String dbKey = entry.getKey();
-          if (dbKey.startsWith(beginKey)) {
-            endKey = dbKey;
-          }
+    // Clean up deletedDirectoryTable
+    deleteRangeInclusive(omMetadataManager.getDeletedDirTable(),
+        beginKey, endKey);
+  }
+
+  /**
+   * Helper method to generate /volumeId/bucketId/ DB key prefix from given
+   * volume name and bucket name as a prefix in FSO deletedDirectoryTable.
+   * Follows:
+   * {@link OmMetadataManagerImpl#getOzonePathKey(long, long, long, String)}.
+   * <p>
+   * Note: Currently, this is only intended to be a special use case in
+   * {@link OmSnapshotManager}. If this is used elsewhere, consider moving this
+   * to {@link OMMetadataManager}.
+   *
+   * @param volumeName volume name
+   * @param bucketName bucket name
+   * @return /volumeId/bucketId/
+   *    e.g. /-9223372036854772480/-9223372036854771968/
+   */
+  @VisibleForTesting
+  public static String getOzonePathKeyWithVolumeBucketNames(
+      OMMetadataManager omMetadataManager,
+      String volumeName,
+      String bucketName) throws IOException {
+
+    final long volumeId = omMetadataManager.getVolumeId(volumeName);
+    final long bucketId = omMetadataManager.getBucketId(volumeName, bucketName);
+    return OM_KEY_PREFIX + volumeId + OM_KEY_PREFIX + bucketId + OM_KEY_PREFIX;
+  }
+
+  /**
+   * Helper method to locate the end key with the given prefix and iterator.
+   * @param keyIter TableIterator
+   * @param keyPrefix DB key prefix String
+   * @return endKey String, or null if no keys with such prefix is found
+   */
+  private static String findEndKeyGivenPrefix(
+      TableIterator<String, ? extends Table.KeyValue<String, ?>> keyIter,
+      String keyPrefix) throws IOException {
+
+    String endKey;
+    keyIter.seek(keyPrefix);
+    // Continue only when there are entries of snapshot (bucket) scope
+    // in deletedTable in the first place
+    if (!keyIter.hasNext()) {
+      // No key matching keyPrefix. No need to do delete or deleteRange at all.
+      endKey = null;
+    } else {
+      // Remember the last key with a matching prefix
+      endKey = keyIter.next().getKey();
+
+      // Loop until prefix mismatches.
+      // TODO: [SNAPSHOT] Try to seek to next predicted bucket name instead of
+      //  the while-loop for a potential speed up?
+      // Start performance tracking timer
+      long startTime = System.nanoTime();
+      while (keyIter.hasNext()) {
+        Table.KeyValue<String, ?> entry = keyIter.next();
+        String dbKey = entry.getKey();
+        if (dbKey.startsWith(keyPrefix)) {
+          endKey = dbKey;
         }
       }
+      // Time took for the iterator to finish (in ns)
+      long timeElapsed = System.nanoTime() - startTime;
+      if (timeElapsed >= DB_TABLE_ITER_LOOP_THRESHOLD_NS) {
+        // Print time elapsed
+        LOG.warn("Took {} ns to find endKey. Caller is {}", timeElapsed,
+            new Throwable().fillInStackTrace().getStackTrace()[1]
+                .getMethodName());
+      }
     }
+    return endKey;
+  }
 
-    // Time took for the iterator to finish (in ns)
-    long timeElapsed = System.nanoTime() - startTime;
-    if (timeElapsed >= DB_TABLE_ITER_LOOP_THRESHOLD_NS) {
-      // Print time elapsed
-      LOG.warn("Took {} ns to clean up deletedTable", timeElapsed);
-    }
+  /**
+   * Helper method to do deleteRange on a table, including endKey.
+   * TODO: Move this into {@link Table} ?
+   * @param table Table
+   * @param beginKey begin key
+   * @param endKey end key
+   */
+  private static void deleteRangeInclusive(
+      Table<String, ?> table, String beginKey, String endKey)
+      throws IOException {
 
     if (endKey != null) {
-      // Clean up deletedTable
-      omMetadataManager.getDeletedTable().deleteRange(beginKey, endKey);
-
+      table.deleteRange(beginKey, endKey);
       // Remove range end key itself
-      omMetadataManager.getDeletedTable().delete(endKey);
+      table.delete(endKey);
     }
+  }
 
-    // Note: We do not need to invalidate deletedTable cache since entries
-    // are not added to its table cache in the first place.
-    // See OMKeyDeleteRequest and OMKeyPurgeRequest#validateAndUpdateCache.
+  /**
+   * Helper method to delete DB keys in the snapshot scope (bucket)
+   * from active DB's deletedTable.
+   * @param omMetadataManager OMMetadataManager instance
+   * @param volumeName volume name
+   * @param bucketName bucket name
+   */
+  private static void deleteKeysFromDelKeyTableInSnapshotScope(
+      OMMetadataManager omMetadataManager,
+      String volumeName,
+      String bucketName) throws IOException {
+
+    // Range delete start key (inclusive)
+    final String beginKey =
+        omMetadataManager.getOzoneKey(volumeName, bucketName, "");
+    // Range delete end key (exclusive). To be found
+    String endKey;
 
+    try (TableIterator<String,
+        ? extends Table.KeyValue<String, RepeatedOmKeyInfo>>
+             iter = omMetadataManager.getDeletedTable().iterator()) {
+      endKey = findEndKeyGivenPrefix(iter, beginKey);
+    }
+
+    // Clean up deletedTable
+    deleteRangeInclusive(omMetadataManager.getDeletedTable(), beginKey, endKey);
+
+    // No need to invalidate deletedTable (or deletedDirectoryTable) table
+    // cache since entries are not added to its table cache in the first place.
+    // See OMKeyDeleteRequest and OMKeyPurgeRequest#validateAndUpdateCache.
+    //
     // This makes the table clean up efficient as we only need one
     // deleteRange() operation. No need to invalidate cache entries
     // one by one.

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java

@@ -108,7 +108,7 @@ public int getPriority() {
     }
 
     @Override
-    public BackgroundTaskResult call() throws Exception {
+    public BackgroundTaskResult call() {
       if (shouldRun()) {
         if (LOG.isDebugEnabled()) {
           LOG.debug("Running DirectoryDeletingService");
@@ -122,6 +122,8 @@ public BackgroundTaskResult call() throws Exception {
         List<Pair<String, OmKeyInfo>> allSubDirList
             = new ArrayList<>((int) remainNum);
 
+        // TODO: [SNAPSHOT] HDDS-8067. Acquire deletedDirectoryTable write lock
+
         Table.KeyValue<String, OmKeyInfo> pendingDeletedDirInfo;
         try (TableIterator<String, ? extends KeyValue<String, OmKeyInfo>>
                  deleteTableIterator = getOzoneManager().getMetadataManager().
@@ -159,6 +161,8 @@ public BackgroundTaskResult call() throws Exception {
           LOG.error("Error while running delete directories and files " +
               "background task. Will retry at next run.", e);
         }
+        // TODO: [SNAPSHOT] HDDS-8067. Release deletedDirectoryTable write lock
+        //  in finally block
       }
 
       // place holder by returning empty results of this call back.

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyDeletingService.java

@@ -113,7 +113,7 @@ public int getPriority() {
     }
 
     @Override
-    public BackgroundTaskResult call() throws Exception {
+    public BackgroundTaskResult call() {
       // Check if this is the Leader OM. If not leader, no need to execute this
       // task.
       if (shouldRun()) {
@@ -125,6 +125,8 @@ public BackgroundTaskResult call() throws Exception {
           //  OM would have to keep track of which snapshot the key is coming
           //  from. And PurgeKeysRequest would have to be adjusted to be able
           //  to operate on snapshot checkpoints.
+
+          // TODO: [SNAPSHOT] HDDS-8067. Acquire deletedTable write lock
           List<BlockGroup> keyBlocksList = manager
               .getPendingDeletionKeys(keyLimitPerTask);
           if (keyBlocksList != null && !keyBlocksList.isEmpty()) {
@@ -136,6 +138,8 @@ public BackgroundTaskResult call() throws Exception {
           LOG.error("Error while running delete keys background task. Will " +
               "retry at next run.", e);
         }
+        // TODO: [SNAPSHOT] HDDS-8067. Release deletedTable write lock
+        //  in finally block
       }
       // By design, no one cares about the results of this call back.
       return EmptyTaskResult.newResult();

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotUtils.java

@@ -63,7 +63,7 @@ public static SnapshotInfo getSnapshotInfo(final OzoneManager ozoneManager,
           .getSnapshotInfoTable()
           .get(snapshotKey);
     } catch (IOException e) {
-      LOG.error("Snapshot {}: not found.", snapshotKey, e);
+      LOG.error("Snapshot '{}' is not found.", snapshotKey, e);
       throw e;
     }
     if (snapshotInfo == null) {

hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java

@@ -26,6 +26,8 @@
 import org.apache.hadoop.hdds.scm.HddsWhiteboxTestUtils;
 import org.apache.hadoop.hdds.utils.db.DBStore;
 import org.apache.hadoop.hdds.utils.db.Table;
+import org.apache.hadoop.ozone.om.helpers.OmBucketInfo;
+import org.apache.hadoop.ozone.om.helpers.OmVolumeArgs;
 import org.apache.hadoop.ozone.om.helpers.SnapshotInfo;
 import org.apache.hadoop.ozone.om.snapshot.OmSnapshotUtils;
 import org.apache.ozone.test.GenericTestUtils;
@@ -47,6 +49,9 @@
 import static org.apache.hadoop.ozone.OzoneConsts.OM_DB_NAME;
 import static org.apache.hadoop.ozone.OzoneConsts.OM_KEY_PREFIX;
 import static org.apache.hadoop.ozone.OzoneConsts.OM_SNAPSHOT_CHECKPOINT_DIR;
+import static org.apache.hadoop.ozone.OzoneConsts.SNAPSHOT_INFO_TABLE;
+import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.BUCKET_TABLE;
+import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.VOLUME_TABLE;
 import static org.apache.hadoop.ozone.om.OmSnapshotManager.OM_HARDLINK_FILE;
 import static org.apache.hadoop.ozone.om.snapshot.OmSnapshotUtils.getINode;
 import static org.apache.hadoop.ozone.om.OmSnapshotManager.getSnapshotPrefix;
@@ -91,14 +96,39 @@ public void cleanup() throws Exception {
   @Test
   public void testCloseOnEviction() throws IOException {
 
-    // set up db table
-    SnapshotInfo first = createSnapshotInfo();
-    SnapshotInfo second = createSnapshotInfo();
+    // set up db tables
+    Table<String, OmVolumeArgs> volumeTable = mock(Table.class);
+    Table<String, OmBucketInfo> bucketTable = mock(Table.class);
     Table<String, SnapshotInfo> snapshotInfoTable = mock(Table.class);
+    HddsWhiteboxTestUtils.setInternalState(
+        om.getMetadataManager(), VOLUME_TABLE, volumeTable);
+    HddsWhiteboxTestUtils.setInternalState(
+        om.getMetadataManager(), BUCKET_TABLE, bucketTable);
+    HddsWhiteboxTestUtils.setInternalState(
+        om.getMetadataManager(), SNAPSHOT_INFO_TABLE, snapshotInfoTable);
+
+    final String volumeName = UUID.randomUUID().toString();
+    final String dbVolumeKey = om.getMetadataManager().getVolumeKey(volumeName);
+    final OmVolumeArgs omVolumeArgs = OmVolumeArgs.newBuilder()
+        .setVolume(volumeName)
+        .setAdminName("bilbo")
+        .setOwnerName("bilbo")
+        .build();
+    when(volumeTable.get(dbVolumeKey)).thenReturn(omVolumeArgs);
+
+    String bucketName = UUID.randomUUID().toString();
+    final String dbBucketKey = om.getMetadataManager().getBucketKey(
+        volumeName, bucketName);
+    final OmBucketInfo omBucketInfo = OmBucketInfo.newBuilder()
+        .setVolumeName(volumeName)
+        .setBucketName(bucketName)
+        .build();
+    when(bucketTable.get(dbBucketKey)).thenReturn(omBucketInfo);
+
+    SnapshotInfo first = createSnapshotInfo(volumeName, bucketName);
+    SnapshotInfo second = createSnapshotInfo(volumeName, bucketName);
     when(snapshotInfoTable.get(first.getTableKey())).thenReturn(first);
     when(snapshotInfoTable.get(second.getTableKey())).thenReturn(second);
-    HddsWhiteboxTestUtils.setInternalState(
-        om.getMetadataManager(), "snapshotInfoTable", snapshotInfoTable);
 
     // create the first snapshot checkpoint
     OmSnapshotManager.createOmSnapshotCheckpoint(om.getMetadataManager(),
@@ -188,15 +218,12 @@ public void testHardLinkCreation() throws IOException {
     }
   }
 
-  private SnapshotInfo createSnapshotInfo() {
+  private SnapshotInfo createSnapshotInfo(
+      String volumeName, String bucketName) {
     String snapshotName = UUID.randomUUID().toString();
-    String volumeName = UUID.randomUUID().toString();
-    String bucketName = UUID.randomUUID().toString();
     String snapshotId = UUID.randomUUID().toString();
-    return SnapshotInfo.newInstance(volumeName,
-        bucketName,
-        snapshotName,
-        snapshotId);
+    return SnapshotInfo.newInstance(
+        volumeName, bucketName, snapshotName, snapshotId);
   }
 
 }