hemantk-12 commented on code in PR #5339:
URL: https://github.com/apache/ozone/pull/5339#discussion_r1338942172
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/SnapshotInfo.java:
##########
@@ -168,7 +169,8 @@ private SnapshotInfo(UUID snapshotId,
long referencedSize,
long referencedReplicatedSize,
long exclusiveSize,
- long exclusiveReplicatedSize) {
+ long exclusiveReplicatedSize,
+ boolean expandedDeletedDir) {
Review Comment:
nit: Please add this to constructor's java doc comment too.
##########
hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmSnapshotInfo.java:
##########
@@ -174,6 +177,8 @@ public void testSnapshotInfoProtoToSnapshotInfo() {
snapshotInfoActual.getExclusiveSize());
Assert.assertEquals(snapshotInfoExpected.getExclusiveReplicatedSize(),
snapshotInfoActual.getExclusiveReplicatedSize());
+ Assert.assertEquals(snapshotInfoExpected.getExpandedDeletedDir(),
Review Comment:
Can we just `assertEquals` on `snapshotInfoExpected` and
`snapshotInfoActual`?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -222,12 +225,122 @@ public BackgroundTaskResult call() {
getOzoneManager().getMetadataManager().getTableLock(
OmMetadataManagerImpl.DELETED_DIR_TABLE).writeLock().unlock();
}
+
+ try {
+ if (remainNum > 0) {
+ expandSnapshotDirectories(remainNum);
+ }
+ } catch (Exception e) {
Review Comment:
I think you can just catch this exception inside `expandSnapshotDirectories`
and log there instead of logging it here.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -222,12 +225,122 @@ public BackgroundTaskResult call() {
getOzoneManager().getMetadataManager().getTableLock(
OmMetadataManagerImpl.DELETED_DIR_TABLE).writeLock().unlock();
}
+
+ try {
+ if (remainNum > 0) {
+ expandSnapshotDirectories(remainNum);
+ }
+ } catch (Exception e) {
+ LOG.error("Error while running deep clean on snapshots. Will " +
+ "retry at next run.", e);
+ }
}
// place holder by returning empty results of this call back.
return BackgroundTaskResult.EmptyTaskResult.newResult();
}
+ private void expandSnapshotDirectories(long remainNum) throws IOException {
+ OmSnapshotManager omSnapshotManager =
+ getOzoneManager().getOmSnapshotManager();
+ Table<String, SnapshotInfo> snapshotInfoTable =
+ getOzoneManager().getMetadataManager().getSnapshotInfoTable();
+
+ long dirNum = 0L;
+ long subDirNum = 0L;
+ long subFileNum = 0L;
+ int consumedSize = 0;
+ List<PurgePathRequest> purgePathRequestList = new ArrayList<>();
+ try (TableIterator<String, ? extends Table.KeyValue
+ <String, SnapshotInfo>> iterator = snapshotInfoTable.iterator()) {
+
+ while (remainNum > 0 && iterator.hasNext()) {
+ SnapshotInfo currSnapInfo = iterator.next().getValue();
+
+ // Expand deleted dirs only on active snapshot. Deleted Snapshots
+ // will be cleaned up by SnapshotDeletingService.
+ if (!currSnapInfo.getSnapshotStatus().equals(SNAPSHOT_ACTIVE) ||
+ currSnapInfo.getExpandedDeletedDir()) {
+ continue;
+ }
+
+ try (ReferenceCounted<IOmMetadataReader, SnapshotCache>
+ rcCurrOmSnapshot = omSnapshotManager.checkForSnapshot(
+ currSnapInfo.getVolumeName(),
+ currSnapInfo.getBucketName(),
+ getSnapshotPrefix(currSnapInfo.getName()),
+ true)) {
+
+ OmSnapshot currOmSnapshot = (OmSnapshot) rcCurrOmSnapshot.get();
+ Table<String, OmKeyInfo> snapDeletedDirTable =
+ currOmSnapshot.getMetadataManager().getDeletedDirTable();
+
+ if (snapDeletedDirTable.isEmpty()) {
+ // TODO: [SNAPSHOT] Update Snapshot state using
+ // SetSnapshotProperty from HDDS-7743 (YET TO BE MERGED)
+ continue;
+ }
+
+ Table.KeyValue<String, OmKeyInfo> deletedDirInfo;
+ List<Pair<String, OmKeyInfo>> allSubDirList
Review Comment:
nit: Is there any specific reason it is list of `Pair`? If no, use map
instead for simplification.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -222,12 +225,122 @@ public BackgroundTaskResult call() {
getOzoneManager().getMetadataManager().getTableLock(
OmMetadataManagerImpl.DELETED_DIR_TABLE).writeLock().unlock();
}
+
+ try {
+ if (remainNum > 0) {
+ expandSnapshotDirectories(remainNum);
+ }
+ } catch (Exception e) {
+ LOG.error("Error while running deep clean on snapshots. Will " +
+ "retry at next run.", e);
+ }
}
// place holder by returning empty results of this call back.
return BackgroundTaskResult.EmptyTaskResult.newResult();
}
+ private void expandSnapshotDirectories(long remainNum) throws IOException {
+ OmSnapshotManager omSnapshotManager =
+ getOzoneManager().getOmSnapshotManager();
+ Table<String, SnapshotInfo> snapshotInfoTable =
+ getOzoneManager().getMetadataManager().getSnapshotInfoTable();
+
+ long dirNum = 0L;
+ long subDirNum = 0L;
+ long subFileNum = 0L;
+ int consumedSize = 0;
+ List<PurgePathRequest> purgePathRequestList = new ArrayList<>();
+ try (TableIterator<String, ? extends Table.KeyValue
+ <String, SnapshotInfo>> iterator = snapshotInfoTable.iterator()) {
+
+ while (remainNum > 0 && iterator.hasNext()) {
+ SnapshotInfo currSnapInfo = iterator.next().getValue();
+
+ // Expand deleted dirs only on active snapshot. Deleted Snapshots
+ // will be cleaned up by SnapshotDeletingService.
+ if (!currSnapInfo.getSnapshotStatus().equals(SNAPSHOT_ACTIVE) ||
+ currSnapInfo.getExpandedDeletedDir()) {
+ continue;
+ }
+
+ try (ReferenceCounted<IOmMetadataReader, SnapshotCache>
+ rcCurrOmSnapshot = omSnapshotManager.checkForSnapshot(
+ currSnapInfo.getVolumeName(),
+ currSnapInfo.getBucketName(),
+ getSnapshotPrefix(currSnapInfo.getName()),
+ true)) {
+
+ OmSnapshot currOmSnapshot = (OmSnapshot) rcCurrOmSnapshot.get();
+ Table<String, OmKeyInfo> snapDeletedDirTable =
+ currOmSnapshot.getMetadataManager().getDeletedDirTable();
+
+ if (snapDeletedDirTable.isEmpty()) {
+ // TODO: [SNAPSHOT] Update Snapshot state using
+ // SetSnapshotProperty from HDDS-7743 (YET TO BE MERGED)
+ continue;
+ }
+
+ Table.KeyValue<String, OmKeyInfo> deletedDirInfo;
+ List<Pair<String, OmKeyInfo>> allSubDirList
+ = new ArrayList<>((int) remainNum);
+
+ try (TableIterator<String, ? extends Table.KeyValue<String,
Review Comment:
Code inside this try block is mostly duplicate of [above try
block](https://github.com/apache/ozone/blob/5f4bbf3cfcf50223bfdbb98033b8f39d51a40450/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java#L170).
Duplication can be removed, if you create a helper function and extract this
out. It will also improve the readability.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -222,12 +225,122 @@ public BackgroundTaskResult call() {
getOzoneManager().getMetadataManager().getTableLock(
OmMetadataManagerImpl.DELETED_DIR_TABLE).writeLock().unlock();
}
+
+ try {
+ if (remainNum > 0) {
+ expandSnapshotDirectories(remainNum);
+ }
+ } catch (Exception e) {
+ LOG.error("Error while running deep clean on snapshots. Will " +
+ "retry at next run.", e);
+ }
}
// place holder by returning empty results of this call back.
return BackgroundTaskResult.EmptyTaskResult.newResult();
}
+ private void expandSnapshotDirectories(long remainNum) throws IOException {
+ OmSnapshotManager omSnapshotManager =
+ getOzoneManager().getOmSnapshotManager();
+ Table<String, SnapshotInfo> snapshotInfoTable =
+ getOzoneManager().getMetadataManager().getSnapshotInfoTable();
+
+ long dirNum = 0L;
+ long subDirNum = 0L;
+ long subFileNum = 0L;
+ int consumedSize = 0;
+ List<PurgePathRequest> purgePathRequestList = new ArrayList<>();
+ try (TableIterator<String, ? extends Table.KeyValue
+ <String, SnapshotInfo>> iterator = snapshotInfoTable.iterator()) {
+
+ while (remainNum > 0 && iterator.hasNext()) {
+ SnapshotInfo currSnapInfo = iterator.next().getValue();
+
+ // Expand deleted dirs only on active snapshot. Deleted Snapshots
+ // will be cleaned up by SnapshotDeletingService.
+ if (!currSnapInfo.getSnapshotStatus().equals(SNAPSHOT_ACTIVE) ||
+ currSnapInfo.getExpandedDeletedDir()) {
+ continue;
+ }
+
+ try (ReferenceCounted<IOmMetadataReader, SnapshotCache>
+ rcCurrOmSnapshot = omSnapshotManager.checkForSnapshot(
+ currSnapInfo.getVolumeName(),
+ currSnapInfo.getBucketName(),
+ getSnapshotPrefix(currSnapInfo.getName()),
+ true)) {
+
+ OmSnapshot currOmSnapshot = (OmSnapshot) rcCurrOmSnapshot.get();
+ Table<String, OmKeyInfo> snapDeletedDirTable =
+ currOmSnapshot.getMetadataManager().getDeletedDirTable();
+
+ if (snapDeletedDirTable.isEmpty()) {
+ // TODO: [SNAPSHOT] Update Snapshot state using
+ // SetSnapshotProperty from HDDS-7743 (YET TO BE MERGED)
+ continue;
+ }
+
+ Table.KeyValue<String, OmKeyInfo> deletedDirInfo;
+ List<Pair<String, OmKeyInfo>> allSubDirList
+ = new ArrayList<>((int) remainNum);
+
+ try (TableIterator<String, ? extends Table.KeyValue<String,
+ OmKeyInfo>> deletedIterator = snapDeletedDirTable.iterator()) {
+
+ long startTime = Time.monotonicNow();
+ deletedDirInfo = deletedIterator.next();
+
+ PurgePathRequest request = prepareDeleteDirRequest(
+ remainNum, deletedDirInfo.getValue(),
+ deletedDirInfo.getKey(), allSubDirList,
+ currOmSnapshot.getKeyManager());
+ if (isBufferLimitCrossed(ratisByteLimit, consumedSize,
+ request.getSerializedSize())) {
+ if (purgePathRequestList.size() != 0) {
+ // if message buffer reaches max limit, avoid sending further
+ remainNum = 0;
+ break;
+ }
+ // if directory itself is having a lot of keys / files,
+ // reduce capacity to minimum level
+ remainNum = MIN_ERR_LIMIT_PER_TASK;
+ request = prepareDeleteDirRequest(
+ remainNum, deletedDirInfo.getValue(),
+ deletedDirInfo.getKey(), allSubDirList,
+ currOmSnapshot.getKeyManager());
+ }
+
+ consumedSize += request.getSerializedSize();
+ purgePathRequestList.add(request);
+ remainNum = remainNum - request.getDeletedSubFilesCount();
+ remainNum = remainNum - request.getMarkDeletedSubDirsCount();
+ // Count up the purgeDeletedDir, subDirs and subFiles
+ if (request.getDeletedDir() != null
Review Comment:
nit: `getDeletedDir()` will produce not-null always if I'm not wrong.
```suggestion
if (request.hasDeletedDir() &&
!request.getDeletedDir().isEmpty()) {
dirNum++;
}
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -222,12 +225,122 @@ public BackgroundTaskResult call() {
getOzoneManager().getMetadataManager().getTableLock(
OmMetadataManagerImpl.DELETED_DIR_TABLE).writeLock().unlock();
}
+
+ try {
+ if (remainNum > 0) {
+ expandSnapshotDirectories(remainNum);
+ }
+ } catch (Exception e) {
+ LOG.error("Error while running deep clean on snapshots. Will " +
+ "retry at next run.", e);
+ }
}
// place holder by returning empty results of this call back.
return BackgroundTaskResult.EmptyTaskResult.newResult();
}
+ private void expandSnapshotDirectories(long remainNum) throws IOException {
+ OmSnapshotManager omSnapshotManager =
+ getOzoneManager().getOmSnapshotManager();
+ Table<String, SnapshotInfo> snapshotInfoTable =
+ getOzoneManager().getMetadataManager().getSnapshotInfoTable();
+
+ long dirNum = 0L;
+ long subDirNum = 0L;
+ long subFileNum = 0L;
+ int consumedSize = 0;
+ List<PurgePathRequest> purgePathRequestList = new ArrayList<>();
+ try (TableIterator<String, ? extends Table.KeyValue
+ <String, SnapshotInfo>> iterator = snapshotInfoTable.iterator()) {
+
+ while (remainNum > 0 && iterator.hasNext()) {
+ SnapshotInfo currSnapInfo = iterator.next().getValue();
+
+ // Expand deleted dirs only on active snapshot. Deleted Snapshots
+ // will be cleaned up by SnapshotDeletingService.
+ if (!currSnapInfo.getSnapshotStatus().equals(SNAPSHOT_ACTIVE) ||
Review Comment:
nit: use `==` and `!=` for enum.
```suggestion
if (currSnapInfo.getSnapshotStatus() != SNAPSHOT_ACTIVE ||
currSnapInfo.getExpandedDeletedDir()) {
```
--
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]
