hemantk-12 commented on code in PR #6965:
URL: https://github.com/apache/ozone/pull/6965#discussion_r1687224493
##########
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) {
Review Comment:
Can we reuse existing
[getSnapshotPath](https://github.com/apache/ozone/blob/a5e420cf90c67728a47f9350340a059bed1ed1c2/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java#L697)?
It is duplicate code.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -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()
Review Comment:
If I'm not wrong, a lock is taken here so that the snapshot dir doesn't get
deleted when the file creation is getting created here. Please add a comment
for it.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -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);
Review Comment:
nit: should we log it at an info level, if SstFileringService fails because
the snapshot is purged? Otherwise, it will be unnecessary noisy logs.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestSnapshotDeletingService.java:
##########
@@ -69,7 +69,7 @@ private static Stream<Arguments>
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),
Review Comment:
nit: should we update this test? Earlier `shouldIgnoreSnapshot()` was
depended on snapshot's status and SstFilteringServicce's enablement. Now it
doesn't depend on `SstFilteringServicce`, we may remove all those cases.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -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<String, SnapshotInfo> 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));
Review Comment:
nit: maybe a warning as well to not delete
[example](https://github.com/apache/ozone/blob/b11b80707d4e6721c06966936ad87063ec107da6/hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java#L275).
--
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]
