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

@@ -54,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;
@@ -367,9 +368,11 @@ public static DBCheckpoint createOmSnapshotCheckpoint(
       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 {
       // Release deletedTable write lock
       omMetadataManager.getTableLock(OmMetadataManagerImpl.DELETED_TABLE)
@@ -403,73 +406,147 @@ 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)}.
+   * @param volumeName volume name
+   * @param bucketName bucket name
+   * @return /volumeId/bucketId/
+   *    e.g. /-9223372036854772480/-9223372036854771968/
+   */
+  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/snapshot/SnapshotUtils.java

@@ -62,7 +62,7 @@ public static SnapshotInfo getSnapshotInfo(final OzoneManager ozoneManager,
           .getSnapshotInfoTable()
           .get(key);
     } catch (IOException e) {
-      LOG.error("Snapshot {}: not found: {}", key, e);
+      LOG.error("Snapshot '{}' is not found", key, 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;
@@ -88,14 +93,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(),
@@ -185,15 +215,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);
   }
 
 }

hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotResponseTestUtil.java

@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package org.apache.hadoop.ozone.om.response.snapshot;
+
+import org.apache.hadoop.hdds.utils.db.cache.CacheKey;
+import org.apache.hadoop.hdds.utils.db.cache.CacheValue;
+import org.apache.hadoop.ozone.om.OMMetadataManager;
+import org.apache.hadoop.ozone.om.helpers.OmBucketInfo;
+import org.apache.hadoop.ozone.om.helpers.OmVolumeArgs;
+
+import java.io.IOException;
+
+/**
+ * Common test utility method(s) shared between
+ * {@link TestOMSnapshotCreateResponse} and
+ * {@link TestOMSnapshotDeleteResponse}.
+ */
+public final class OMSnapshotResponseTestUtil {
+
+  private OMSnapshotResponseTestUtil() {
+    throw new IllegalStateException("Util class should not be initialized.");
+  }
+
+  static void addVolumeBucketInfoToTable(OMMetadataManager omMetadataManager,
+                                         String volumeName,
+                                         String bucketName)
+      throws IOException {
+
+    // Add volume entry to volumeTable for volumeId lookup
+    final OmVolumeArgs omVolumeArgs = OmVolumeArgs.newBuilder()
+        .setVolume(volumeName)
+        .setAdminName("bilbo")
+        .setOwnerName("bilbo")
+        .build();
+    final String dbVolumeKey = omMetadataManager.getVolumeKey(volumeName);
+    omMetadataManager.getVolumeTable().addCacheEntry(
+        new CacheKey<>(dbVolumeKey),
+        CacheValue.get(1L, omVolumeArgs));
+    omMetadataManager.getVolumeTable().put(dbVolumeKey, omVolumeArgs);
+
+    // Add bucket entry to bucketTable for bucketId lookup
+    final OmBucketInfo omBucketInfo = OmBucketInfo.newBuilder()
+        .setVolumeName(volumeName)
+        .setBucketName(bucketName)
+        .build();
+    final String dbBucketKey = omMetadataManager.getBucketKey(
+        volumeName, bucketName);
+    omMetadataManager.getBucketTable().addCacheEntry(
+        new CacheKey<>(dbBucketKey),
+        CacheValue.get(2L, omBucketInfo));
+    omMetadataManager.getBucketTable().put(dbBucketKey, omBucketInfo);
+  }
+}