smengcl commented on code in PR #4543:
URL: https://github.com/apache/ozone/pull/4543#discussion_r1159266196
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -1712,6 +1713,7 @@ message SnapshotMoveDeletedKeysRequest {
repeated SnapshotMoveKeyInfos nextDBKeys = 2;
repeated SnapshotMoveKeyInfos reclaimKeys = 3;
repeated hadoop.hdds.KeyValue renamedKeys = 4;
+ repeated string dirsToMove = 5;
Review Comment:
Make it more specific
```suggestion
repeated string deletedDirKeysToMove = 5;
```
Or `deletedDirsToMove`
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -138,27 +113,33 @@ public BackgroundTaskResult call() throws Exception {
if (LOG.isDebugEnabled()) {
LOG.debug("Running DirectoryDeletingService");
}
- runCount.incrementAndGet();
+ getRunCount().incrementAndGet();
int dirNum = 0;
int subDirNum = 0;
int subFileNum = 0;
long remainNum = pathLimitPerTask;
List<PurgePathRequest> purgePathRequestList = new ArrayList<>();
+ List<Pair<String, OmKeyInfo>> allSubDirList
+ = new ArrayList<>((int) remainNum);
Table.KeyValue<String, OmKeyInfo> pendingDeletedDirInfo;
try (TableIterator<String, ? extends KeyValue<String, OmKeyInfo>>
- deleteTableIterator = ozoneManager.getMetadataManager().
+ deleteTableIterator = getOzoneManager().getMetadataManager().
getDeletedDirTable().iterator()) {
- List<Pair<String, OmKeyInfo>> allSubDirList
- = new ArrayList<>((int) remainNum);
long startTime = Time.monotonicNow();
while (remainNum > 0 && deleteTableIterator.hasNext()) {
pendingDeletedDirInfo = deleteTableIterator.next();
+ // Do not reclaim if the directory is still being referenced by
+ // the previous snapshot.
+ if (checkDirPartOfPreviousSnapshot(pendingDeletedDirInfo)) {
Review Comment:
```suggestion
if (previousSnapshotHasDir(pendingDeletedDirInfo)) {
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java:
##########
@@ -58,6 +63,9 @@ public abstract class AbstractKeyDeletingService extends
BackgroundService {
private final OzoneManager ozoneManager;
private final ScmBlockLocationProtocol scmClient;
private static ClientId clientId = ClientId.randomId();
+ private final AtomicLong deletedDirsCount;
+ private final AtomicLong movedDirsCount;
+ private final AtomicLong movedFilesCount;
Review Comment:
Actually these can just be `Long` if they are not shared among threads. Each
KDT/DDT thread would have its own set of counters.
If you need these to be shared you'd have to add `static` keyword and
initialize it in-place or in a static initializer.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotMoveDeletedKeysResponse.java:
##########
@@ -98,6 +103,22 @@ protected void addToDBBatch(OMMetadataManager
omMetadataManager,
}
}
+ private void processDirs(BatchOperation batchOp,
+ OMMetadataManager omMetadataManager)
+ throws IOException {
+ for (String movedDirsKey : movedDirs) {
+ OmKeyInfo keyInfo =
fromSnapshot.getMetadataManager().getDeletedDirTable()
+ .get(movedDirsKey);
+ // Move deleted dirs to next snapshot or active DB
+ omMetadataManager.getDeletedDirTable().putWithBatch(
+ batchOp, movedDirsKey, keyInfo);
+ // Delete dirs from current snapshot that are moved to next snapshot.
+ fromSnapshot.getMetadataManager().getDeletedDirTable()
+ .deleteWithBatch(batchOp, movedDirsKey);
+ }
+ }
+
+
Review Comment:
```suggestion
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -227,165 +164,32 @@ public BackgroundTaskResult call() throws Exception {
// place holder by returning empty results of this call back.
return BackgroundTaskResult.EmptyTaskResult.newResult();
}
- }
-
- private PurgePathRequest prepareDeleteDirRequest(
- long remainNum, OmKeyInfo pendingDeletedDirInfo, String delDirName,
- List<Pair<String, OmKeyInfo>> subDirList) throws IOException {
- // step-0: Get one pending deleted directory
- if (LOG.isDebugEnabled()) {
- LOG.debug("Pending deleted dir name: {}",
- pendingDeletedDirInfo.getKeyName());
- }
-
- final String[] keys = delDirName.split(OM_KEY_PREFIX);
- final long volumeId = Long.parseLong(keys[1]);
- final long bucketId = Long.parseLong(keys[2]);
-
- // step-1: get all sub directories under the deletedDir
- List<OmKeyInfo> subDirs = ozoneManager.getKeyManager()
- .getPendingDeletionSubDirs(volumeId, bucketId,
- pendingDeletedDirInfo, remainNum);
- remainNum = remainNum - subDirs.size();
-
- OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
- for (OmKeyInfo dirInfo : subDirs) {
- String ozoneDbKey = omMetadataManager.getOzonePathKey(volumeId,
- bucketId, dirInfo.getParentObjectID(), dirInfo.getFileName());
- String ozoneDeleteKey = omMetadataManager.getOzoneDeletePathKey(
- dirInfo.getObjectID(), ozoneDbKey);
- subDirList.add(Pair.of(ozoneDeleteKey, dirInfo));
- LOG.debug("Moved sub dir name: {}", dirInfo.getKeyName());
- }
-
- // step-2: get all sub files under the deletedDir
- List<OmKeyInfo> subFiles = ozoneManager.getKeyManager()
- .getPendingDeletionSubFiles(volumeId, bucketId,
- pendingDeletedDirInfo, remainNum);
- remainNum = remainNum - subFiles.size();
- if (LOG.isDebugEnabled()) {
- for (OmKeyInfo fileInfo : subFiles) {
- LOG.debug("Moved sub file name: {}", fileInfo.getKeyName());
+ private boolean checkDirPartOfPreviousSnapshot(
+ KeyValue<String, OmKeyInfo> pendingDeletedDirInfo) throws IOException {
+ String key = pendingDeletedDirInfo.getKey();
+ OmKeyInfo deletedDirInfo = pendingDeletedDirInfo.getValue();
+ OmSnapshotManager omSnapshotManager =
+ getOzoneManager().getOmSnapshotManager();
+ OmMetadataManagerImpl metadataManager = (OmMetadataManagerImpl)
+ getOzoneManager().getMetadataManager();
+
+ OmSnapshot latestSnapshot =
+ metadataManager.getLatestSnapshot(deletedDirInfo.getVolumeName(),
+ deletedDirInfo.getBucketName(), omSnapshotManager);
+
+ if (latestSnapshot != null) {
+ Table<String, OmDirectoryInfo> prevDirTable =
+ latestSnapshot.getMetadataManager().getDirectoryTable();
+ // In OMKeyDeleteResponseWithFSO OzonePathKey is converted to
+ // OzoneDeletePathKey. Changing it back to check the previous DirTable.
+ String prevDbKey = metadataManager.getOzoneDeletePathToOzonePath(key);
+ OmDirectoryInfo prevDirInfo = prevDirTable.get(prevDbKey);
+ return prevDirInfo != null &&
+ prevDirInfo.getObjectID() == prevDirInfo.getObjectID();
Review Comment:
typo?
```suggestion
return prevDirInfo != null &&
pendingDeletedDirInfo.getObjectID() == prevDirInfo.getObjectID();
```
##########
hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/OMMetadataManager.java:
##########
@@ -466,6 +466,15 @@ default String getOzonePathKey(long volumeId, long
bucketId,
*/
String getOzoneDeletePathKey(long objectId, String pathKey);
+ /**
+ * Given ozone delete path key return the corresponding
+ * DB path key for directory table.
+ *
+ * @param ozoneDeletePath - ozone delete path
+ * @return DB directory key as String.
+ */
+ String getOzoneDeletePathToOzonePath(String ozoneDeletePath);
Review Comment:
name?
```suggestion
String getOzoneDeletePathDirKey(String ozoneDeletePath);
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -172,9 +174,21 @@ public BackgroundTaskResult call() throws Exception {
" is not found", BUCKET_NOT_FOUND);
Review Comment:
We can throw `IllegalStateException` instead since `bucketInfo` should not
be null under normal circumstances.
And at least make the error message very very clear that OM is not in an
expected state.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java:
##########
Review Comment:
Add to the class javadoc that this abstract class has both
`KeyDeletingService` and `DirDeletingService` common logic now.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -257,20 +276,19 @@ public BackgroundTaskResult call() throws Exception {
}
deletionCount++;
}
- // Submit Move request to OM.
- submitSnapshotMoveDeletedKeys(snapInfo, toReclaimList,
- toNextDBList, renamedKeysList);
// Delete keys From deletedTable
processKeyDeletes(keysToPurge, omSnapshot.getKeyManager(),
snapInfo.getTableKey());
- snapshotLimit--;
successRunCount.incrementAndGet();
} catch (IOException ex) {
LOG.error("Error while running Snapshot Deleting Service", ex);
}
Review Comment:
Print more helpful messages in this case. e.g. which snapshotId, how many
keys and dirs processed at this point
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -279,6 +297,87 @@ public BackgroundTaskResult call() throws Exception {
return BackgroundTaskResult.EmptyTaskResult.newResult();
}
+ private boolean checkSnapshotReclaimable(
+ Table<String, RepeatedOmKeyInfo> snapshotDeletedTable,
+ Table<String, OmKeyInfo> snapshotDeletedDirTable,
+ String snapshotBucketKey, String dbBucketKeyForDir) throws IOException
{
+
+ boolean isDirTableCleanedUp = false;
+ boolean isKeyTableCleanedUp = false;
+ try (TableIterator<String, ? extends Table.KeyValue<String,
+ RepeatedOmKeyInfo>> iterator = snapshotDeletedTable.iterator();) {
+ iterator.seek(snapshotBucketKey);
+ isKeyTableCleanedUp = iterator.hasNext() && iterator.next().getKey()
+ .startsWith(snapshotBucketKey);
+ }
+
+ try (TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
+ iterator = snapshotDeletedDirTable.iterator()) {
+ iterator.seek(dbBucketKeyForDir);
+ isDirTableCleanedUp = iterator.hasNext() && iterator.next().getKey()
+ .startsWith(dbBucketKeyForDir);
+ }
+
+ return (isDirTableCleanedUp || snapshotDeletedDirTable.isEmpty()) &&
+ (isKeyTableCleanedUp || snapshotDeletedTable.isEmpty());
+ }
+
+ private long handleDirectoryCleanUp(
+ Table<String, OmKeyInfo> snapshotDeletedDirTable,
+ Table<String, OmDirectoryInfo> previousDirTable,
+ String dbBucketKeyForDir,
+ SnapshotInfo snapInfo, OmSnapshot omSnapshot,
+ List<String> dirsToMove) {
+
+ int dirNum = 0;
+ int subDirNum = 0;
+ int subFileNum = 0;
+ long remainNum = keyLimitPerSnapshot;
+ List<PurgePathRequest> purgePathRequestList = new ArrayList<>();
+ List<Pair<String, OmKeyInfo>> allSubDirList
+ = new ArrayList<>((int) remainNum);
Review Comment:
one less type conversion
```suggestion
List<Pair<String, OmKeyInfo>> allSubDirList
= new ArrayList<>(keyLimitPerSnapshot);
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -187,25 +201,32 @@ public BackgroundTaskResult call() throws Exception {
previousKeyTable = omPreviousSnapshot
.getMetadataManager().getKeyTable(bucketInfo.getBucketLayout());
+ previousDirTable = omPreviousSnapshot
+ .getMetadataManager().getDirectoryTable();
}
// Move key to either next non deleted snapshot's deletedTable
// or keep it in current snapshot deleted table.
List<SnapshotMoveKeyInfos> toReclaimList = new ArrayList<>();
List<SnapshotMoveKeyInfos> toNextDBList = new ArrayList<>();
List<HddsProtos.KeyValue> renamedKeysList = new ArrayList<>();
+ List<String> dirsToMove = new ArrayList<>();
+
+ long remainNum = handleDirectoryCleanUp(snapshotDeletedDirTable,
+ previousDirTable, dbBucketKeyForDir, snapInfo, omSnapshot,
+ dirsToMove);
+
Review Comment:
```suggestion
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -279,6 +297,87 @@ public BackgroundTaskResult call() throws Exception {
return BackgroundTaskResult.EmptyTaskResult.newResult();
}
+ private boolean checkSnapshotReclaimable(
+ Table<String, RepeatedOmKeyInfo> snapshotDeletedTable,
+ Table<String, OmKeyInfo> snapshotDeletedDirTable,
+ String snapshotBucketKey, String dbBucketKeyForDir) throws IOException
{
+
+ boolean isDirTableCleanedUp = false;
+ boolean isKeyTableCleanedUp = false;
+ try (TableIterator<String, ? extends Table.KeyValue<String,
+ RepeatedOmKeyInfo>> iterator = snapshotDeletedTable.iterator();) {
+ iterator.seek(snapshotBucketKey);
+ isKeyTableCleanedUp = iterator.hasNext() && iterator.next().getKey()
+ .startsWith(snapshotBucketKey);
+ }
+
+ try (TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
+ iterator = snapshotDeletedDirTable.iterator()) {
+ iterator.seek(dbBucketKeyForDir);
+ isDirTableCleanedUp = iterator.hasNext() && iterator.next().getKey()
+ .startsWith(dbBucketKeyForDir);
+ }
+
+ return (isDirTableCleanedUp || snapshotDeletedDirTable.isEmpty()) &&
+ (isKeyTableCleanedUp || snapshotDeletedTable.isEmpty());
+ }
+
+ private long handleDirectoryCleanUp(
+ Table<String, OmKeyInfo> snapshotDeletedDirTable,
+ Table<String, OmDirectoryInfo> previousDirTable,
+ String dbBucketKeyForDir,
+ SnapshotInfo snapInfo, OmSnapshot omSnapshot,
+ List<String> dirsToMove) {
+
+ int dirNum = 0;
+ int subDirNum = 0;
+ int subFileNum = 0;
+ long remainNum = keyLimitPerSnapshot;
Review Comment:
consistency?
```suggestion
long dirNum = 0L;
long subDirNum = 0L;
long subFileNum = 0L;
long remainNum = keyLimitPerSnapshot;
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -279,6 +297,87 @@ public BackgroundTaskResult call() throws Exception {
return BackgroundTaskResult.EmptyTaskResult.newResult();
}
+ private boolean checkSnapshotReclaimable(
+ Table<String, RepeatedOmKeyInfo> snapshotDeletedTable,
+ Table<String, OmKeyInfo> snapshotDeletedDirTable,
+ String snapshotBucketKey, String dbBucketKeyForDir) throws IOException
{
+
+ boolean isDirTableCleanedUp = false;
+ boolean isKeyTableCleanedUp = false;
+ try (TableIterator<String, ? extends Table.KeyValue<String,
+ RepeatedOmKeyInfo>> iterator = snapshotDeletedTable.iterator();) {
+ iterator.seek(snapshotBucketKey);
+ isKeyTableCleanedUp = iterator.hasNext() && iterator.next().getKey()
+ .startsWith(snapshotBucketKey);
+ }
+
+ try (TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
+ iterator = snapshotDeletedDirTable.iterator()) {
+ iterator.seek(dbBucketKeyForDir);
+ isDirTableCleanedUp = iterator.hasNext() && iterator.next().getKey()
+ .startsWith(dbBucketKeyForDir);
+ }
+
+ return (isDirTableCleanedUp || snapshotDeletedDirTable.isEmpty()) &&
+ (isKeyTableCleanedUp || snapshotDeletedTable.isEmpty());
+ }
+
+ private long handleDirectoryCleanUp(
+ Table<String, OmKeyInfo> snapshotDeletedDirTable,
+ Table<String, OmDirectoryInfo> previousDirTable,
+ String dbBucketKeyForDir,
+ SnapshotInfo snapInfo, OmSnapshot omSnapshot,
+ List<String> dirsToMove) {
+
+ int dirNum = 0;
+ int subDirNum = 0;
+ int subFileNum = 0;
+ long remainNum = keyLimitPerSnapshot;
+ List<PurgePathRequest> purgePathRequestList = new ArrayList<>();
+ List<Pair<String, OmKeyInfo>> allSubDirList
+ = new ArrayList<>((int) remainNum);
+ try (TableIterator<String, ? extends
+ Table.KeyValue<String, OmKeyInfo>> deletedDirIterator =
+ snapshotDeletedDirTable.iterator()) {
+
+ long startTime = Time.monotonicNow();
+ deletedDirIterator.seek(dbBucketKeyForDir);
+
+ while (deletedDirIterator.hasNext()) {
+ Table.KeyValue<String, OmKeyInfo> deletedDir =
+ deletedDirIterator.next();
+
+ if (checkDirReclaimable(deletedDir, previousDirTable)) {
+ // Reclaim here
+ PurgePathRequest request = prepareDeleteDirRequest(
+ remainNum, deletedDir.getValue(), deletedDir.getKey(),
+ allSubDirList, omSnapshot.getKeyManager());
+ purgePathRequestList.add(request);
+ remainNum = remainNum - request.getDeletedSubFilesCount();
+ remainNum = remainNum - request.getMarkDeletedSubDirsCount();
+ // Count up the purgeDeletedDir, subDirs and subFiles
+ if (request.getDeletedDir() != null
+ && !request.getDeletedDir().isEmpty()) {
+ dirNum++;
+ }
+ subDirNum += request.getMarkDeletedSubDirsCount();
+ subFileNum += request.getDeletedSubFilesCount();
+ } else {
+ dirsToMove.add(deletedDir.getKey());
+ }
+ }
+
+ remainNum = optimizeDirDeletesAndSubmitRequest(remainNum, dirNum,
+ subDirNum, subFileNum, allSubDirList, purgePathRequestList,
+ snapInfo.getTableKey(), startTime);
+ } catch (IOException e) {
+ LOG.error("Error while running delete directories and files in" +
+ "Snapshot deleting background task. Will retry at next run.", e);
Review Comment:
```suggestion
LOG.error("Error while running delete directories and files in " +
"snapshot deleting background task. Will retry at next run.", e);
```
--
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]
