smengcl commented on code in PR #4407:
URL: https://github.com/apache/ozone/pull/4407#discussion_r1139847694
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -182,13 +199,18 @@ public BackgroundTaskResult call() throws Exception {
String snapshotBucketKey = dbBucketKey + OzoneConsts.OM_KEY_PREFIX;
iterator.seek(snapshotBucketKey);
- while (deletedIterator.hasNext()) {
+ int deletionCount = 0;
+ while (deletedIterator.hasNext() &&
+ deletionCount <= keyLimitPerSnapshot) {
Review Comment:
We could add a block comment right above the loop explaining that we are
trying to gather as much key blocks as possible in one SCM block deletion
request here.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -281,12 +331,40 @@ private boolean checkKeyReclaimable(
return false;
}
- //TODO: [SNAPSHOT] Handle Renamed Keys
- String dbKey = ozoneManager.getMetadataManager()
- .getOzoneKey(deletedKeyInfo.getVolumeName(),
- deletedKeyInfo.getBucketName(), deletedKeyInfo.getKeyName());
+ if (bucketInfo.getBucketLayout().isFileSystemOptimized()) {
+ // Handle FSO buckets
+ dbKey = ozoneManager.getMetadataManager().getOzonePathKey(volumeId,
+ bucketInfo.getObjectID(), deletedKeyInfo.getParentObjectID(),
+ deletedKeyInfo.getKeyName());
+ } else {
+ dbKey = ozoneManager.getMetadataManager()
+ .getOzoneKey(deletedKeyInfo.getVolumeName(),
+ deletedKeyInfo.getBucketName(), deletedKeyInfo.getKeyName());
+ }
+
+ // renamedKeyTable: volumeName/bucketName/objectID -> OMRenameKeyInfo
+ String dbRenameKey = ozoneManager.getMetadataManager().getRenameKey(
+ deletedKeyInfo.getVolumeName(), deletedKeyInfo.getBucketName(),
+ deletedKeyInfo.getObjectID());
+
+ OmKeyRenameInfo renamedKeyInfo = renamedKeyTable.getIfExist(dbRenameKey);
+
+ boolean isKeyRenamed = false;
+ String dbOriginalKey = null;
+ // Condition: key should not exist in renamedKeyTable of the current
+ // snapshot and keyTable of the previous snapshot.
+ // Check key exists in renamedKeyTable of the Snapshot
+ if (renamedKeyInfo != null && !renamedKeyInfo
+ .getOmKeyRenameInfoList().isEmpty()) {
+ isKeyRenamed = true;
+ dbOriginalKey = renamedKeyInfo.getOmKeyRenameInfoList().get(0);
+ }
+
+ // previousKeyTable is fileTable if the bucket is FSO,
+ // otherwise it is the keyTable.
+ OmKeyInfo prevKeyInfo = isKeyRenamed ? previousKeyTable
+ .get(dbOriginalKey) : previousKeyTable.get(dbKey);
- OmKeyInfo prevKeyInfo = previousKeyTable.get(dbKey);
if (prevKeyInfo != null &&
Review Comment:
The return here can be changed to the following to make it more readable:
```
if (prevKeyInfo == null) {
return false;
}
return prevKeyInfo.getObjectID() == deletedKeyInfo.getObjectID();
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -92,14 +98,19 @@ public SnapshotDeletingService(long interval, long
serviceTimeout,
serviceTimeout);
this.ozoneManager = ozoneManager;
this.omSnapshotManager = ozoneManager.getOmSnapshotManager();
- this.chainManager = ozoneManager.getSnapshotChainManager();
+ OmMetadataManagerImpl omMetadataManager = (OmMetadataManagerImpl)
+ ozoneManager.getMetadataManager();
+ this.chainManager = omMetadataManager.getSnapshotChainManager();
this.runCount = new AtomicLong(0);
this.successRunCount = new AtomicLong(0);
this.suspended = new AtomicBoolean(false);
this.conf = ozoneManager.getConfiguration();
this.snapshotDeletionPerTask = conf
.getLong(SNAPSHOT_DELETING_LIMIT_PER_TASK,
SNAPSHOT_DELETING_LIMIT_PER_TASK_DEFAULT);
+ this.keyLimitPerSnapshot = conf.getInt(
+ OZONE_SNAPSHOT_KEY_DELETING_LIMIT_PER_TASK,
+ OZONE_SNAPSHOT_KEY_DELETING_LIMIT_PER_TASK_DEFAULT);
}
private class SnapshotDeletingTask implements BackgroundTask {
Review Comment:
Ideally we need new test cases in `TestSnapshotDeletingService` to cover the
rename test cases here.
Or you can file a separate jira for the test case additions.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -281,12 +331,40 @@ private boolean checkKeyReclaimable(
return false;
}
- //TODO: [SNAPSHOT] Handle Renamed Keys
- String dbKey = ozoneManager.getMetadataManager()
- .getOzoneKey(deletedKeyInfo.getVolumeName(),
- deletedKeyInfo.getBucketName(), deletedKeyInfo.getKeyName());
+ if (bucketInfo.getBucketLayout().isFileSystemOptimized()) {
+ // Handle FSO buckets
+ dbKey = ozoneManager.getMetadataManager().getOzonePathKey(volumeId,
+ bucketInfo.getObjectID(), deletedKeyInfo.getParentObjectID(),
+ deletedKeyInfo.getKeyName());
+ } else {
+ dbKey = ozoneManager.getMetadataManager()
+ .getOzoneKey(deletedKeyInfo.getVolumeName(),
+ deletedKeyInfo.getBucketName(), deletedKeyInfo.getKeyName());
+ }
+
+ // renamedKeyTable: volumeName/bucketName/objectID -> OMRenameKeyInfo
+ String dbRenameKey = ozoneManager.getMetadataManager().getRenameKey(
+ deletedKeyInfo.getVolumeName(), deletedKeyInfo.getBucketName(),
+ deletedKeyInfo.getObjectID());
+
+ OmKeyRenameInfo renamedKeyInfo = renamedKeyTable.getIfExist(dbRenameKey);
+
+ boolean isKeyRenamed = false;
+ String dbOriginalKey = null;
+ // Condition: key should not exist in renamedKeyTable of the current
+ // snapshot and keyTable of the previous snapshot.
+ // Check key exists in renamedKeyTable of the Snapshot
+ if (renamedKeyInfo != null && !renamedKeyInfo
+ .getOmKeyRenameInfoList().isEmpty()) {
Review Comment:
We can refactor renamedKeyTable to keep a single `String` in its value. Can
be done in HDDS-8189 as well.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -281,12 +331,40 @@ private boolean checkKeyReclaimable(
return false;
}
- //TODO: [SNAPSHOT] Handle Renamed Keys
- String dbKey = ozoneManager.getMetadataManager()
- .getOzoneKey(deletedKeyInfo.getVolumeName(),
- deletedKeyInfo.getBucketName(), deletedKeyInfo.getKeyName());
+ if (bucketInfo.getBucketLayout().isFileSystemOptimized()) {
+ // Handle FSO buckets
+ dbKey = ozoneManager.getMetadataManager().getOzonePathKey(volumeId,
+ bucketInfo.getObjectID(), deletedKeyInfo.getParentObjectID(),
+ deletedKeyInfo.getKeyName());
+ } else {
+ dbKey = ozoneManager.getMetadataManager()
+ .getOzoneKey(deletedKeyInfo.getVolumeName(),
+ deletedKeyInfo.getBucketName(), deletedKeyInfo.getKeyName());
+ }
Review Comment:
nit
```suggestion
// Construct keyTable or fileTable DB key depending on the bucket type
if (bucketInfo.getBucketLayout().isFileSystemOptimized()) {
dbKey = ozoneManager.getMetadataManager().getOzonePathKey(
volumeId,
bucketInfo.getObjectID(),
deletedKeyInfo.getParentObjectID(),
deletedKeyInfo.getKeyName());
} else {
dbKey = ozoneManager.getMetadataManager().getOzoneKey(
deletedKeyInfo.getVolumeName(),
deletedKeyInfo.getBucketName(),
deletedKeyInfo.getKeyName());
}
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -182,13 +199,18 @@ public BackgroundTaskResult call() throws Exception {
String snapshotBucketKey = dbBucketKey + OzoneConsts.OM_KEY_PREFIX;
iterator.seek(snapshotBucketKey);
- while (deletedIterator.hasNext()) {
+ int deletionCount = 0;
+ while (deletedIterator.hasNext() &&
+ deletionCount <= keyLimitPerSnapshot) {
Table.KeyValue<String, RepeatedOmKeyInfo>
deletedKeyValue = deletedIterator.next();
String deletedKey = deletedKeyValue.getKey();
// Exit if it is out of the bucket scope.
if (!deletedKey.startsWith(snapshotBucketKey)) {
+ // If snapshot deletedKeyTable doesn't have any
+ // entry in the snapshot scope it can be reclaimed
+ purgeSnapshotKeys.add(snapInfo.getTableKey());
break;
}
Review Comment:
We haven't checked `deletedDirTable` emptiness right? Or add a TODO here
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -281,12 +331,40 @@ private boolean checkKeyReclaimable(
return false;
Review Comment:
This means we won't reclaim those blocks (e.g. added by Hsync) in
deletedTable? while actually the blocks **could** be deleted if the block ID
doesn't match.
This may deserve a TODO here (Add block ID checking). Also, add more
comments regarding the desired GC behavior.
Make sure when deferring this entry to the next snapshot or active DB, it is
merged properly into the same OmKeyInfo. The blocks cannot be thrown away
without reclamation (leaking blocks on SCMs/DNs otherwise).
--
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]
