hemantk-12 commented on code in PR #6965:
URL: https://github.com/apache/ozone/pull/6965#discussion_r1695825160
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java:
##########
@@ -116,15 +117,23 @@ 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);
+ // 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.
Review Comment:
```suggestion
// inside the snapshot directory. Any operation apart from SSTFiltering
can run in parallel with this operation.
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -69,6 +73,10 @@ 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 static final byte[] SST_FILTERED_FILE_CONTENT =
StringUtils.string2Bytes("This directory holds information " +
Review Comment:
```suggestion
private static final byte[] SST_FILTERED_FILE_CONTENT =
StringUtils.string2Bytes("This file holds information " +
"if a particular snapshot has filtered out the irrelevant sst files or
not.\nDO NOT add, change or delete " +
"this file unless you know what you are doing.\n");
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -114,31 +128,28 @@ 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 {
+ // 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.
Review Comment:
nit:
```suggestion
// in OmSnapshotPurgeResponse. Any operation apart from delete can run
in parallel with this operation.
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -69,6 +73,10 @@ 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";
Review Comment:
nit: should we have `.txt` or any other text file extension?
##########
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:
Yes, that's why I asked to log it at the info level if it fails because
snapshot dir doesn't exist which will be the case when it is purged. RocksDB
throws exception like `org.rocksdb.RocksDBException: While mkdir if missing:
path_to_db_dir: No such file or directory.`.
--
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]
