From 25d178f92d51a7b8b38393844b01e7698b836dc2 Mon Sep 17 00:00:00 2001 From: Swaminathan Balachandran Date: Thu, 18 Jul 2024 20:37:31 -0700 Subject: [PATCH 1/6] HDDS-11068. Move SstFiltered flag to a file in the snapshot directory Change-Id: I9e16fe4b117c232423c1d2de3f48d99acb71827f --- .../TestSnapshotBackgroundServices.java | 3 +- .../hadoop/ozone/om/OmSnapshotManager.java | 7 +++ .../hadoop/ozone/om/SstFilteringService.java | 49 ++++++++++--------- .../snapshot/OMSnapshotPurgeResponse.java | 27 ++++++---- .../om/service/SnapshotDeletingService.java | 6 +-- .../service/TestSnapshotDeletingService.java | 2 +- .../om/snapshot/TestSstFilteringService.java | 7 +-- 7 files changed, 59 insertions(+), 42 deletions(-) diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotBackgroundServices.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotBackgroundServices.java index 2f7e1bd5a9d6..afae893e7b97 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotBackgroundServices.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotBackgroundServices.java @@ -38,6 +38,7 @@ import org.apache.hadoop.ozone.om.OmFailoverProxyUtil; import org.apache.hadoop.ozone.om.OmSnapshot; import org.apache.hadoop.ozone.om.OzoneManager; +import org.apache.hadoop.ozone.om.SstFilteringService; import org.apache.hadoop.ozone.om.exceptions.OMLeaderNotReadyException; import org.apache.hadoop.ozone.om.exceptions.OMNotLeaderException; import org.apache.hadoop.ozone.om.helpers.BucketLayout; @@ -572,7 +573,7 @@ private void checkIfSnapshotGetsProcessedBySFS(OzoneManager ozoneManager) } catch (IOException e) { fail(); } - return snapshotInfo.isSstFiltered(); + return SstFilteringService.isSstFiltered(ozoneManager.getMetadataManager(), snapshotInfo); }, 1000, 10000); } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java index 636b2594e0c4..0b6e3a90faa9 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java @@ -441,6 +441,13 @@ public void invalidateCacheEntry(UUID key) { } } + public static Path getSnapshotPath(OMMetadataManager omMetadataManager, SnapshotInfo snapshotInfo) { + RDBStore store = (RDBStore) omMetadataManager.getStore(); + String checkpointPrefix = store.getDbLocation().getName(); + return Paths.get(store.getSnapshotsParentDir(), + checkpointPrefix + snapshotInfo.getCheckpointDir()); + } + /** * Creates snapshot checkpoint that corresponds to snapshotInfo. * @param omMetadataManager the metadata manager diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java index 20d0ab0e53eb..04a82aa58aae 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java @@ -38,6 +38,10 @@ import org.slf4j.LoggerFactory; import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; import java.util.Map; import java.util.Optional; import java.util.concurrent.TimeUnit; @@ -69,6 +73,7 @@ public class SstFilteringService extends BackgroundService // multiple times. private static final int SST_FILTERING_CORE_POOL_SIZE = 1; + public static final String SST_FILTERED_FILE = "sstFiltered"; private final OzoneManager ozoneManager; // Number of files to be batched in an iteration. @@ -78,6 +83,12 @@ public class SstFilteringService extends BackgroundService private AtomicBoolean running; + public static boolean isSstFiltered(OMMetadataManager omMetadataManager, SnapshotInfo snapshotInfo) { + Path sstFilteredFile = Paths.get(OmSnapshotManager.getSnapshotPath(omMetadataManager, + snapshotInfo).toFile().getAbsolutePath(), SST_FILTERED_FILE); + return snapshotInfo.isSstFiltered() || sstFilteredFile.toFile().exists(); + } + public SstFilteringService(long interval, TimeUnit unit, long serviceTimeout, OzoneManager ozoneManager, OzoneConfiguration configuration) { super("SstFilteringService", interval, unit, SST_FILTERING_CORE_POOL_SIZE, @@ -114,31 +125,25 @@ private class SstFilteringTask implements BackgroundTask { /** - * Marks the SSTFiltered flag corresponding to the snapshot. - * @param volume Volume name of the snapshot - * @param bucket Bucket name of the snapshot - * @param snapshotName Snapshot name + * Marks the snapshot as SSTFiltered by creating a file in snapshot directory. + * @param snapshotInfo snapshotInfo * @throws IOException */ - private void markSSTFilteredFlagForSnapshot(String volume, String bucket, - String snapshotName) throws IOException { + private void markSSTFilteredFlagForSnapshot(SnapshotInfo snapshotInfo) throws IOException { OMLockDetails omLockDetails = ozoneManager.getMetadataManager().getLock() - .acquireWriteLock(SNAPSHOT_LOCK, volume, bucket, snapshotName); + .acquireWriteLock(SNAPSHOT_LOCK, snapshotInfo.getVolumeName(), + snapshotInfo.getBucketName(), snapshotInfo.getName()); boolean acquiredSnapshotLock = omLockDetails.isLockAcquired(); if (acquiredSnapshotLock) { - Table snapshotInfoTable = - ozoneManager.getMetadataManager().getSnapshotInfoTable(); + Path snapshotDir = OmSnapshotManager.getSnapshotPath(ozoneManager.getMetadataManager(), snapshotInfo); try { - // mark the snapshot as filtered by writing to the file - String snapshotTableKey = SnapshotInfo.getTableKey(volume, bucket, - snapshotName); - SnapshotInfo snapshotInfo = snapshotInfoTable.get(snapshotTableKey); - - snapshotInfo.setSstFiltered(true); - snapshotInfoTable.put(snapshotTableKey, snapshotInfo); + // mark the snapshot as filtered by creating a file. + Files.write(Paths.get(snapshotDir.toFile().getAbsolutePath(), SST_FILTERED_FILE), + Long.toString(System.currentTimeMillis()).getBytes(StandardCharsets.UTF_8)); } finally { ozoneManager.getMetadataManager().getLock() - .releaseWriteLock(SNAPSHOT_LOCK, volume, bucket, snapshotName); + .releaseWriteLock(SNAPSHOT_LOCK, snapshotInfo.getVolumeName(), + snapshotInfo.getBucketName(), snapshotInfo.getName()); } } } @@ -167,8 +172,7 @@ public BackgroundTaskResult call() throws Exception { Table.KeyValue keyValue = iterator.next(); String snapShotTableKey = keyValue.getKey(); SnapshotInfo snapshotInfo = keyValue.getValue(); - - if (snapshotInfo.isSstFiltered()) { + if (isSstFiltered(ozoneManager.getMetadataManager(), snapshotInfo)) { continue; } @@ -194,6 +198,9 @@ public BackgroundTaskResult call() throws Exception { .lock()) { db.deleteFilesNotMatchingPrefix(columnFamilyNameToPrefixMap); } + markSSTFilteredFlagForSnapshot(snapshotInfo); + snapshotLimit--; + snapshotFilteredCount.getAndIncrement(); } catch (OMException ome) { // FILE_NOT_FOUND is obtained when the snapshot is deleted // In this case, get the snapshotInfo from the db, check if @@ -210,10 +217,6 @@ public BackgroundTaskResult call() throws Exception { } } } - markSSTFilteredFlagForSnapshot(snapshotInfo.getVolumeName(), - snapshotInfo.getBucketName(), snapshotInfo.getName()); - snapshotLimit--; - snapshotFilteredCount.getAndIncrement(); } catch (RocksDBException | IOException e) { LOG.error("Exception encountered while filtering a snapshot", e); } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java index d300601b3858..ff84a9a0116e 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java @@ -23,7 +23,9 @@ import org.apache.hadoop.hdds.utils.db.RDBStore; import org.apache.hadoop.ozone.om.OMMetadataManager; import org.apache.hadoop.ozone.om.OmMetadataManagerImpl; +import org.apache.hadoop.ozone.om.OmSnapshotManager; import org.apache.hadoop.ozone.om.helpers.SnapshotInfo; +import org.apache.hadoop.ozone.om.lock.OMLockDetails; import org.apache.hadoop.ozone.om.response.CleanupTableInfo; import org.apache.hadoop.ozone.om.response.OMClientResponse; import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMResponse; @@ -38,6 +40,7 @@ import java.util.Map; import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.SNAPSHOT_INFO_TABLE; +import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.SNAPSHOT_LOCK; /** * Response for OMSnapshotPurgeRequest. @@ -116,15 +119,21 @@ private void updateSnapInfo(OmMetadataManagerImpl metadataManager, */ private void deleteCheckpointDirectory(OMMetadataManager omMetadataManager, SnapshotInfo snapshotInfo) { - RDBStore store = (RDBStore) omMetadataManager.getStore(); - String checkpointPrefix = store.getDbLocation().getName(); - Path snapshotDirPath = Paths.get(store.getSnapshotsParentDir(), - checkpointPrefix + snapshotInfo.getCheckpointDir()); - try { - FileUtils.deleteDirectory(snapshotDirPath.toFile()); - } catch (IOException ex) { - LOG.error("Failed to delete snapshot directory {} for snapshot {}", - snapshotDirPath, snapshotInfo.getTableKey(), ex); + OMLockDetails omLockDetails = omMetadataManager.getLock() + .acquireWriteLock(SNAPSHOT_LOCK, snapshotInfo.getVolumeName(), snapshotInfo.getBucketName(), + snapshotInfo.getName()); + boolean acquiredSnapshotLock = omLockDetails.isLockAcquired(); + if (acquiredSnapshotLock) { + Path snapshotDirPath = OmSnapshotManager.getSnapshotPath(omMetadataManager, snapshotInfo); + try { + FileUtils.deleteDirectory(snapshotDirPath.toFile()); + } catch (IOException ex) { + LOG.error("Failed to delete snapshot directory {} for snapshot {}", + snapshotDirPath, snapshotInfo.getTableKey(), ex); + } finally { + omMetadataManager.getLock().releaseWriteLock(SNAPSHOT_LOCK, snapshotInfo.getVolumeName(), + snapshotInfo.getBucketName(), snapshotInfo.getName()); + } } } } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java index bb4fc076bdee..21d9ad25f236 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java @@ -100,7 +100,6 @@ public class SnapshotDeletingService extends AbstractKeyDeletingService { private final long snapshotDeletionPerTask; private final int keyLimitPerSnapshot; private final int ratisByteLimit; - private final boolean isSstFilteringServiceEnabled; public SnapshotDeletingService(long interval, long serviceTimeout, OzoneManager ozoneManager, ScmBlockLocationProtocol scmClient) @@ -128,8 +127,6 @@ public SnapshotDeletingService(long interval, long serviceTimeout, this.keyLimitPerSnapshot = conf.getInt( OZONE_SNAPSHOT_KEY_DELETING_LIMIT_PER_TASK, OZONE_SNAPSHOT_KEY_DELETING_LIMIT_PER_TASK_DEFAULT); - - this.isSstFilteringServiceEnabled = ((KeyManagerImpl) ozoneManager.getKeyManager()).isSstFilteringSvcEnabled(); } private class SnapshotDeletingTask implements BackgroundTask { @@ -594,8 +591,7 @@ public void submitRequest(OMRequest omRequest) { @VisibleForTesting boolean shouldIgnoreSnapshot(SnapshotInfo snapInfo) { SnapshotInfo.SnapshotStatus snapshotStatus = snapInfo.getSnapshotStatus(); - return snapshotStatus != SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED - || (isSstFilteringServiceEnabled && !snapInfo.isSstFiltered()); + return snapshotStatus != SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED; } // TODO: Move this util class. diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestSnapshotDeletingService.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestSnapshotDeletingService.java index 42da7377ea26..d0bd45b44196 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestSnapshotDeletingService.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestSnapshotDeletingService.java @@ -69,7 +69,7 @@ private static Stream testCasesForIgnoreSnapshotGc() { return Stream.of( Arguments.of(filteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, true, false), Arguments.of(filteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true, true), - Arguments.of(unFilteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, true, true), + Arguments.of(unFilteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, true, false), Arguments.of(unFilteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true, true), Arguments.of(filteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false, false), Arguments.of(unFilteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false, false), diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSstFilteringService.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSstFilteringService.java index 31ca16481f4c..d65a358e45d9 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSstFilteringService.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSstFilteringService.java @@ -206,7 +206,7 @@ public void testIrrelevantSstFileDeletion() createSnapshot(volumeName, bucketName2, snapshotName1); SnapshotInfo snapshotInfo = om.getMetadataManager().getSnapshotInfoTable() .get(SnapshotInfo.getTableKey(volumeName, bucketName2, snapshotName1)); - assertFalse(snapshotInfo.isSstFiltered()); + assertFalse(SstFilteringService.isSstFiltered(om.getMetadataManager(), snapshotInfo)); waitForSnapshotsAtLeast(filteringService, countExistingSnapshots + 1); assertEquals(countExistingSnapshots + 1, filteringService.getSnapshotFilteredCount().get()); @@ -238,8 +238,9 @@ public void testIrrelevantSstFileDeletion() // Need to read the sstFiltered flag which is set in background process and // hence snapshotInfo.isSstFiltered() may not work sometimes. - assertTrue(om.getMetadataManager().getSnapshotInfoTable().get(SnapshotInfo - .getTableKey(volumeName, bucketName2, snapshotName1)).isSstFiltered()); + assertTrue(SstFilteringService.isSstFiltered(om.getMetadataManager(), + om.getMetadataManager().getSnapshotInfoTable().get(SnapshotInfo + .getTableKey(volumeName, bucketName2, snapshotName1)))); String snapshotName2 = "snapshot2"; final long count; From b0f7f6b590264d79e8e57f26b3d8dc75e0fef4e2 Mon Sep 17 00:00:00 2001 From: Swaminathan Balachandran Date: Thu, 18 Jul 2024 22:01:39 -0700 Subject: [PATCH 2/6] HDDS-11068. Fix checkstyle Change-Id: I36cee730a30496264b32c74a5f25ae2e92936eba --- .../ozone/om/response/snapshot/OMSnapshotPurgeResponse.java | 2 -- .../apache/hadoop/ozone/om/service/SnapshotDeletingService.java | 1 - 2 files changed, 3 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java index ff84a9a0116e..f42ef06d083e 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java @@ -20,7 +20,6 @@ import org.apache.commons.io.FileUtils; import org.apache.hadoop.hdds.utils.db.BatchOperation; -import org.apache.hadoop.hdds.utils.db.RDBStore; import org.apache.hadoop.ozone.om.OMMetadataManager; import org.apache.hadoop.ozone.om.OmMetadataManagerImpl; import org.apache.hadoop.ozone.om.OmSnapshotManager; @@ -35,7 +34,6 @@ import jakarta.annotation.Nonnull; import java.io.IOException; import java.nio.file.Path; -import java.nio.file.Paths; import java.util.List; import java.util.Map; diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java index 21d9ad25f236..99e3903447d4 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java @@ -36,7 +36,6 @@ import org.apache.hadoop.ozone.lock.BootstrapStateHandler; import org.apache.hadoop.ozone.om.OMConfigKeys; import org.apache.hadoop.ozone.om.OmMetadataManagerImpl; -import org.apache.hadoop.ozone.om.KeyManagerImpl; import org.apache.hadoop.ozone.om.OmSnapshot; import org.apache.hadoop.ozone.om.OmSnapshotManager; import org.apache.hadoop.ozone.om.OzoneManager; From 71884e05b1555f3dbcb4f63d4bc9a9fb3c21e3fd Mon Sep 17 00:00:00 2001 From: Swaminathan Balachandran Date: Fri, 19 Jul 2024 12:51:50 -0700 Subject: [PATCH 3/6] HDDS-11068. Fix tests Change-Id: Id5e3ae98555b3102e24bdfb3fb38e2e7e24c26a8 --- .../java/org/apache/hadoop/ozone/om/OMMetadataManager.java | 3 +-- .../hadoop/ozone/om/snapshot/TestSstFilteringService.java | 7 ++++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/OMMetadataManager.java b/hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/OMMetadataManager.java index fd64bb274d98..2e207f1a32c4 100644 --- a/hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/OMMetadataManager.java +++ b/hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/OMMetadataManager.java @@ -46,8 +46,7 @@ import org.apache.hadoop.hdds.utils.TransactionInfo; import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.ExpiredMultipartUploadsBucket; import org.apache.hadoop.ozone.snapshot.ListSnapshotResponse; -import org.apache.hadoop.ozone.storage.proto. - OzoneManagerStorageProtos.PersistedUserVolumeInfo; +import org.apache.hadoop.ozone.storage.proto.OzoneManagerStorageProtos.PersistedUserVolumeInfo; import org.apache.hadoop.ozone.security.OzoneTokenIdentifier; import org.apache.hadoop.hdds.utils.db.DBStore; import org.apache.hadoop.hdds.utils.db.Table; diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSstFilteringService.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSstFilteringService.java index d65a358e45d9..73ad84d0e990 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSstFilteringService.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSstFilteringService.java @@ -314,7 +314,7 @@ public void testActiveAndDeletedSnapshotCleanup() throws Exception { .filter(f -> f.getName().endsWith(SST_FILE_EXTENSION)).count(); // delete snap1 - writeClient.deleteSnapshot(volumeName, bucketNames.get(0), "snap1"); + deleteSnapshot(volumeName, bucketNames.get(0), "snap1"); sstFilteringService.resume(); // Filtering service will only act on snap2 as it is an active snaphot waitForSnapshotsAtLeast(sstFilteringService, countTotalSnapshots); @@ -506,4 +506,9 @@ private void createSnapshot(String volumeName, String bucketName, String snapsho writeClient.createSnapshot(volumeName, bucketName, snapshotName); countTotalSnapshots++; } + + private void deleteSnapshot(String volumeName, String bucketName, String snapshotName) throws IOException { + writeClient.deleteSnapshot(volumeName, bucketName, snapshotName); + countTotalSnapshots--; + } } From 4723c2e48bc470862e9185e8938e70a4faf71676 Mon Sep 17 00:00:00 2001 From: Swaminathan Balachandran Date: Fri, 26 Jul 2024 18:55:09 -0700 Subject: [PATCH 4/6] HDDS-11068. Address review comments Change-Id: I438ede5cd999cb991064cf9f4bf142568b03f98f --- .../TestSnapshotBackgroundServices.java | 2 +- .../hadoop/ozone/om/OmSnapshotManager.java | 14 +++++----- .../hadoop/ozone/om/SstFilteringService.java | 26 ++++++++++++------- .../snapshot/OMSnapshotPurgeResponse.java | 2 ++ .../service/TestSnapshotDeletingService.java | 19 ++++++-------- .../om/snapshot/TestSstFilteringService.java | 4 +-- 6 files changed, 36 insertions(+), 31 deletions(-) diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotBackgroundServices.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotBackgroundServices.java index afae893e7b97..cc5bca4d3100 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotBackgroundServices.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotBackgroundServices.java @@ -573,7 +573,7 @@ private void checkIfSnapshotGetsProcessedBySFS(OzoneManager ozoneManager) } catch (IOException e) { fail(); } - return SstFilteringService.isSstFiltered(ozoneManager.getMetadataManager(), snapshotInfo); + return SstFilteringService.isSstFiltered(ozoneManager.getConfiguration(), snapshotInfo); }, 1000, 10000); } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java index 0b6e3a90faa9..0d17851ed1f7 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java @@ -441,13 +441,6 @@ public void invalidateCacheEntry(UUID key) { } } - public static Path getSnapshotPath(OMMetadataManager omMetadataManager, SnapshotInfo snapshotInfo) { - RDBStore store = (RDBStore) omMetadataManager.getStore(); - String checkpointPrefix = store.getDbLocation().getName(); - return Paths.get(store.getSnapshotsParentDir(), - checkpointPrefix + snapshotInfo.getCheckpointDir()); - } - /** * Creates snapshot checkpoint that corresponds to snapshotInfo. * @param omMetadataManager the metadata manager @@ -701,6 +694,13 @@ public static String getSnapshotPrefix(String snapshotName) { snapshotName + OM_KEY_PREFIX; } + public static Path getSnapshotPath(OMMetadataManager omMetadataManager, SnapshotInfo snapshotInfo) { + RDBStore store = (RDBStore) omMetadataManager.getStore(); + String checkpointPrefix = store.getDbLocation().getName(); + return Paths.get(store.getSnapshotsParentDir(), + checkpointPrefix + snapshotInfo.getCheckpointDir()); + } + public static String getSnapshotPath(OzoneConfiguration conf, SnapshotInfo snapshotInfo) { return OMStorage.getOmDbDir(conf) + diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java index 04a82aa58aae..32374ae8e31a 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java @@ -19,6 +19,7 @@ package org.apache.hadoop.ozone.om; import com.google.common.annotations.VisibleForTesting; +import org.apache.hadoop.hdds.StringUtils; import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.utils.BackgroundService; import org.apache.hadoop.hdds.utils.BackgroundTask; @@ -38,7 +39,6 @@ import org.slf4j.LoggerFactory; import java.io.IOException; -import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; @@ -74,6 +74,9 @@ public class SstFilteringService extends BackgroundService private static final int SST_FILTERING_CORE_POOL_SIZE = 1; public static final String SST_FILTERED_FILE = "sstFiltered"; + private static final byte[] SST_FILTERED_FILE_CONTENT = StringUtils.string2Bytes("This directory holds information " + + "if a particular snapshot has filtered out the relevant sst files or not.\nDO NOT add, change or delete " + + "any files in this directory unless you know what you are doing.\n"); private final OzoneManager ozoneManager; // Number of files to be batched in an iteration. @@ -83,9 +86,9 @@ public class SstFilteringService extends BackgroundService private AtomicBoolean running; - public static boolean isSstFiltered(OMMetadataManager omMetadataManager, SnapshotInfo snapshotInfo) { - Path sstFilteredFile = Paths.get(OmSnapshotManager.getSnapshotPath(omMetadataManager, - snapshotInfo).toFile().getAbsolutePath(), SST_FILTERED_FILE); + public static boolean isSstFiltered(OzoneConfiguration ozoneConfiguration, SnapshotInfo snapshotInfo) { + Path sstFilteredFile = Paths.get(OmSnapshotManager.getSnapshotPath(ozoneConfiguration, + snapshotInfo), SST_FILTERED_FILE); return snapshotInfo.isSstFiltered() || sstFilteredFile.toFile().exists(); } @@ -130,16 +133,19 @@ private class SstFilteringTask implements BackgroundTask { * @throws IOException */ private void markSSTFilteredFlagForSnapshot(SnapshotInfo snapshotInfo) throws IOException { + // Acquiring read lock to avoid race condition with the snapshot directory deletion occuring + // in OmSnapshotPurgeResponse. Any operation apart from delete can run in parallel along with operation. OMLockDetails omLockDetails = ozoneManager.getMetadataManager().getLock() - .acquireWriteLock(SNAPSHOT_LOCK, snapshotInfo.getVolumeName(), - snapshotInfo.getBucketName(), snapshotInfo.getName()); + .acquireReadLock(SNAPSHOT_LOCK, snapshotInfo.getVolumeName(), snapshotInfo.getBucketName(), + snapshotInfo.getName()); boolean acquiredSnapshotLock = omLockDetails.isLockAcquired(); if (acquiredSnapshotLock) { - Path snapshotDir = OmSnapshotManager.getSnapshotPath(ozoneManager.getMetadataManager(), snapshotInfo); + String snapshotDir = OmSnapshotManager.getSnapshotPath(ozoneManager.getConfiguration(), snapshotInfo); try { // mark the snapshot as filtered by creating a file. - Files.write(Paths.get(snapshotDir.toFile().getAbsolutePath(), SST_FILTERED_FILE), - Long.toString(System.currentTimeMillis()).getBytes(StandardCharsets.UTF_8)); + if (Files.exists(Paths.get(snapshotDir))) { + Files.write(Paths.get(snapshotDir, SST_FILTERED_FILE), SST_FILTERED_FILE_CONTENT); + } } finally { ozoneManager.getMetadataManager().getLock() .releaseWriteLock(SNAPSHOT_LOCK, snapshotInfo.getVolumeName(), @@ -172,7 +178,7 @@ public BackgroundTaskResult call() throws Exception { Table.KeyValue keyValue = iterator.next(); String snapShotTableKey = keyValue.getKey(); SnapshotInfo snapshotInfo = keyValue.getValue(); - if (isSstFiltered(ozoneManager.getMetadataManager(), snapshotInfo)) { + if (isSstFiltered(ozoneManager.getConfiguration(), snapshotInfo)) { continue; } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java index f42ef06d083e..bd3ec8853303 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java @@ -117,6 +117,8 @@ private void updateSnapInfo(OmMetadataManagerImpl metadataManager, */ private void deleteCheckpointDirectory(OMMetadataManager omMetadataManager, SnapshotInfo snapshotInfo) { + // Acquiring write lock to avoid race condition with sst filtering service which creates a sst filtered file + // inside the snapshot directory. Any operation apart from delete can run in parallel along with operation. OMLockDetails omLockDetails = omMetadataManager.getLock() .acquireWriteLock(SNAPSHOT_LOCK, snapshotInfo.getVolumeName(), snapshotInfo.getBucketName(), snapshotInfo.getName()); diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestSnapshotDeletingService.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestSnapshotDeletingService.java index d0bd45b44196..3948f4fab805 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestSnapshotDeletingService.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestSnapshotDeletingService.java @@ -67,26 +67,23 @@ private static Stream testCasesForIgnoreSnapshotGc() { SnapshotInfo filteredSnapshot = SnapshotInfo.newBuilder().setSstFiltered(true).setName("snap1").build(); SnapshotInfo unFilteredSnapshot = SnapshotInfo.newBuilder().setSstFiltered(false).setName("snap1").build(); return Stream.of( - Arguments.of(filteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, true, false), - Arguments.of(filteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true, true), - Arguments.of(unFilteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, true, false), - Arguments.of(unFilteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true, true), - Arguments.of(filteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false, false), - Arguments.of(unFilteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false, false), - Arguments.of(unFilteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, false, true), - Arguments.of(filteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, false, true)); + Arguments.of(filteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false), + Arguments.of(filteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true), + Arguments.of(unFilteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false), + Arguments.of(unFilteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true), + Arguments.of(filteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false), + Arguments.of(unFilteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED, false), + Arguments.of(unFilteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true), + Arguments.of(filteredSnapshot, SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, true)); } @ParameterizedTest @MethodSource("testCasesForIgnoreSnapshotGc") public void testProcessSnapshotLogicInSDS(SnapshotInfo snapshotInfo, SnapshotInfo.SnapshotStatus status, - boolean sstFilteringServiceEnabled, boolean expectedOutcome) throws IOException { - Mockito.when(keyManager.isSstFilteringSvcEnabled()).thenReturn(sstFilteringServiceEnabled); Mockito.when(omMetadataManager.getSnapshotChainManager()).thenReturn(chainManager); - Mockito.when(ozoneManager.getKeyManager()).thenReturn(keyManager); Mockito.when(ozoneManager.getOmSnapshotManager()).thenReturn(omSnapshotManager); Mockito.when(ozoneManager.getMetadataManager()).thenReturn(omMetadataManager); Mockito.when(ozoneManager.getConfiguration()).thenReturn(conf); diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSstFilteringService.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSstFilteringService.java index 73ad84d0e990..58da4303608b 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSstFilteringService.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSstFilteringService.java @@ -206,7 +206,7 @@ public void testIrrelevantSstFileDeletion() createSnapshot(volumeName, bucketName2, snapshotName1); SnapshotInfo snapshotInfo = om.getMetadataManager().getSnapshotInfoTable() .get(SnapshotInfo.getTableKey(volumeName, bucketName2, snapshotName1)); - assertFalse(SstFilteringService.isSstFiltered(om.getMetadataManager(), snapshotInfo)); + assertFalse(SstFilteringService.isSstFiltered(om.getConfiguration(), snapshotInfo)); waitForSnapshotsAtLeast(filteringService, countExistingSnapshots + 1); assertEquals(countExistingSnapshots + 1, filteringService.getSnapshotFilteredCount().get()); @@ -238,7 +238,7 @@ public void testIrrelevantSstFileDeletion() // Need to read the sstFiltered flag which is set in background process and // hence snapshotInfo.isSstFiltered() may not work sometimes. - assertTrue(SstFilteringService.isSstFiltered(om.getMetadataManager(), + assertTrue(SstFilteringService.isSstFiltered(om.getConfiguration(), om.getMetadataManager().getSnapshotInfoTable().get(SnapshotInfo .getTableKey(volumeName, bucketName2, snapshotName1)))); From d091dbf9162a5b8496e3c77fa62f24584d39cb76 Mon Sep 17 00:00:00 2001 From: Swaminathan Balachandran Date: Wed, 31 Jul 2024 00:36:34 -0700 Subject: [PATCH 5/6] HDDS-11068. Fix comments Change-Id: I373acfefbf65812665794dfeb292a928900b6c73 --- .../hadoop/ozone/om/SstFilteringService.java | 30 ++++++++++++------- .../snapshot/OMSnapshotPurgeResponse.java | 3 +- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java index 32374ae8e31a..6cdf8c1dfd30 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java @@ -74,7 +74,7 @@ public class SstFilteringService extends BackgroundService private static final int SST_FILTERING_CORE_POOL_SIZE = 1; public static final String SST_FILTERED_FILE = "sstFiltered"; - private static final byte[] SST_FILTERED_FILE_CONTENT = StringUtils.string2Bytes("This directory holds information " + + private static final byte[] SST_FILTERED_FILE_CONTENT = StringUtils.string2Bytes("This file holds information " + "if a particular snapshot has filtered out the relevant sst files or not.\nDO NOT add, change or delete " + "any files in this directory unless you know what you are doing.\n"); private final OzoneManager ozoneManager; @@ -126,6 +126,10 @@ public void resume() { private class SstFilteringTask implements BackgroundTask { + private boolean isSnapshotDeleted(SnapshotInfo snapshotInfo) { + return snapshotInfo == null || snapshotInfo.getSnapshotStatus() == SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED; + } + /** * Marks the snapshot as SSTFiltered by creating a file in snapshot directory. @@ -133,8 +137,8 @@ private class SstFilteringTask implements BackgroundTask { * @throws IOException */ private void markSSTFilteredFlagForSnapshot(SnapshotInfo snapshotInfo) throws IOException { - // Acquiring read lock to avoid race condition with the snapshot directory deletion occuring - // in OmSnapshotPurgeResponse. Any operation apart from delete can run in parallel along with operation. + // Acquiring read lock to avoid race condition with the snapshot directory deletion occurring + // in OmSnapshotPurgeResponse. Any operation apart from delete can run in parallel along with this operation. OMLockDetails omLockDetails = ozoneManager.getMetadataManager().getLock() .acquireReadLock(SNAPSHOT_LOCK, snapshotInfo.getVolumeName(), snapshotInfo.getBucketName(), snapshotInfo.getName()); @@ -148,7 +152,7 @@ private void markSSTFilteredFlagForSnapshot(SnapshotInfo snapshotInfo) throws IO } } finally { ozoneManager.getMetadataManager().getLock() - .releaseWriteLock(SNAPSHOT_LOCK, snapshotInfo.getVolumeName(), + .releaseReadLock(SNAPSHOT_LOCK, snapshotInfo.getVolumeName(), snapshotInfo.getBucketName(), snapshotInfo.getName()); } } @@ -174,10 +178,10 @@ public BackgroundTaskResult call() throws Exception { long snapshotLimit = snapshotLimitPerTask; while (iterator.hasNext() && snapshotLimit > 0 && running.get()) { + Table.KeyValue keyValue = iterator.next(); + String snapShotTableKey = keyValue.getKey(); + SnapshotInfo snapshotInfo = keyValue.getValue(); try { - Table.KeyValue keyValue = iterator.next(); - String snapShotTableKey = keyValue.getKey(); - SnapshotInfo snapshotInfo = keyValue.getValue(); if (isSstFiltered(ozoneManager.getConfiguration(), snapshotInfo)) { continue; } @@ -215,8 +219,7 @@ public BackgroundTaskResult call() throws Exception { SnapshotInfo snapshotInfoToCheck = ozoneManager.getMetadataManager().getSnapshotInfoTable() .get(snapShotTableKey); - if (snapshotInfoToCheck.getSnapshotStatus() == - SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED) { + if (isSnapshotDeleted(snapshotInfoToCheck)) { LOG.info("Snapshot with name: '{}', id: '{}' has been " + "deleted.", snapshotInfo.getName(), snapshotInfo .getSnapshotId()); @@ -224,7 +227,14 @@ public BackgroundTaskResult call() throws Exception { } } } catch (RocksDBException | IOException e) { - LOG.error("Exception encountered while filtering a snapshot", e); + if (isSnapshotDeleted(snapshotInfoTable.get(snapShotTableKey))) { + LOG.info("Exception encountered while filtering a snapshot: {} since it was deleted midway", + snapShotTableKey, e); + } else { + LOG.error("Exception encountered while filtering a snapshot", e); + } + + } } } catch (IOException e) { diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java index bd3ec8853303..45b0c5e05909 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java @@ -118,7 +118,8 @@ private void updateSnapInfo(OmMetadataManagerImpl metadataManager, private void deleteCheckpointDirectory(OMMetadataManager omMetadataManager, SnapshotInfo snapshotInfo) { // Acquiring write lock to avoid race condition with sst filtering service which creates a sst filtered file - // inside the snapshot directory. Any operation apart from delete can run in parallel along with operation. + // inside the snapshot directory. Any operation apart which doesn't create/delete files under this snapshot + // directory can run in parallel along with this operation. OMLockDetails omLockDetails = omMetadataManager.getLock() .acquireWriteLock(SNAPSHOT_LOCK, snapshotInfo.getVolumeName(), snapshotInfo.getBucketName(), snapshotInfo.getName()); From 6423746c9c0c3f58aba69aa30aced4a27a94e807 Mon Sep 17 00:00:00 2001 From: Swaminathan Balachandran Date: Wed, 31 Jul 2024 15:13:37 -0700 Subject: [PATCH 6/6] HDDS-11068. Fix comments Change-Id: I73029fede1d667b70c68c11527398216e458f8ae --- .../java/org/apache/hadoop/ozone/om/SstFilteringService.java | 1 + 1 file changed, 1 insertion(+) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java index 6cdf8c1dfd30..52e5da504038 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java @@ -139,6 +139,7 @@ private boolean isSnapshotDeleted(SnapshotInfo snapshotInfo) { private void markSSTFilteredFlagForSnapshot(SnapshotInfo snapshotInfo) throws IOException { // Acquiring read lock to avoid race condition with the snapshot directory deletion occurring // in OmSnapshotPurgeResponse. Any operation apart from delete can run in parallel along with this operation. + //TODO. Revisit other SNAPSHOT_LOCK and see if we can change write locks to read locks to further optimize it. OMLockDetails omLockDetails = ozoneManager.getMetadataManager().getLock() .acquireReadLock(SNAPSHOT_LOCK, snapshotInfo.getVolumeName(), snapshotInfo.getBucketName(), snapshotInfo.getName());