smengcl commented on code in PR #4486:
URL: https://github.com/apache/ozone/pull/4486#discussion_r1153964722
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1395,16 +1395,35 @@ private PersistedUserVolumeInfo getVolumesByUser(String
userNameKey)
}
}
- @Override
- public List<BlockGroup> getPendingDeletionKeys(final int keyCount)
- throws IOException {
+ /**
+ * Returns a list of pending deletion key info that ups to the given count.
Review Comment:
```suggestion
* Returns a list of pending deletion key info up to the limit.
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1423,12 +1442,32 @@ public List<BlockGroup> getPendingDeletionKeys(final
int keyCount)
// 4. Further optimization: Skip all snapshotted keys altogether
// e.g. by prefixing all unreclaimable keys, then calling seek
- // If the bucket is a snapshot bucket then we should not process
- // entries related to the snapshot from the deletedTable.
- boolean isSnapshotBucket =
- OMClientRequestUtils.isSnapshotBucket(this, info);
- if (isSnapshotBucket) {
- break;
+ // TODO: [SNAPSHOT] We are not cleaning up Active DB's deletedTable
+ // keys if any of the keys in RepeatedOmKeyInfo exists
+ // in latest snapshot path keyTable.
Review Comment:
Move comment.
```suggestion
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1394,16 +1395,35 @@ private PersistedUserVolumeInfo getVolumesByUser(String
userNameKey)
}
}
- @Override
- public List<BlockGroup> getPendingDeletionKeys(final int keyCount)
- throws IOException {
+ /**
+ * Returns a list of pending deletion key info that ups to the given count.
+ * Each entry is a {@link BlockGroup}, which contains the info about the key
+ * name and all its associated block IDs. A pending deletion key is stored
+ * with #deleting# prefix in OM DB.
+ *
+ * @param keyCount max number of keys to return.
+ * @param omSnapshotManager SnapshotManager
+ * @return a list of {@link BlockGroup} represent keys and blocks.
+ * @throws IOException
+ */
+ public List<BlockGroup> getPendingDeletionKeys(final int keyCount,
+ OmSnapshotManager omSnapshotManager) throws IOException {
Review Comment:
I think we need more tests in `TestKeyDeletingService` to cover the new code
paths in this method.
e.g. a test case where a new key is created then deleted **after** snapshot
creation, KDS should be able to pick it up and reclaim it.
Can file a jira for this. Add more test scenarios.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1423,12 +1442,32 @@ public List<BlockGroup> getPendingDeletionKeys(final
int keyCount)
// 4. Further optimization: Skip all snapshotted keys altogether
// e.g. by prefixing all unreclaimable keys, then calling seek
- // If the bucket is a snapshot bucket then we should not process
- // entries related to the snapshot from the deletedTable.
- boolean isSnapshotBucket =
- OMClientRequestUtils.isSnapshotBucket(this, info);
- if (isSnapshotBucket) {
- break;
+ // TODO: [SNAPSHOT] We are not cleaning up Active DB's deletedTable
+ // keys if any of the keys in RepeatedOmKeyInfo exists
+ // in latest snapshot path keyTable.
+ if (latestSnapshot != null) {
+ Table<String, OmKeyInfo> prevKeyTable =
+ latestSnapshot.getMetadataManager().getKeyTable(
+ bucketInfo.getBucketLayout());
+ String prevDbKey;
+ if (bucketInfo.getBucketLayout().isFileSystemOptimized()) {
+ long volumeId = getVolumeId(info.getVolumeName());
+ prevDbKey = getOzonePathKey(volumeId,
+ bucketInfo.getObjectID(),
+ info.getParentObjectID(),
+ info.getKeyName());
+ } else {
+ prevDbKey = getOzoneKey(info.getVolumeName(),
+ info.getBucketName(),
+ info.getKeyName());
+ }
+
+ OmKeyInfo omKeyInfo = prevKeyTable.get(prevDbKey);
+ if (omKeyInfo != null &&
+ info.getObjectID() == omKeyInfo.getObjectID()) {
+ blockGroupList.clear();
+ break;
+ }
Review Comment:
Edited and moved here.
```suggestion
if (omKeyInfo != null &&
info.getObjectID() == omKeyInfo.getObjectID()) {
// TODO: [SNAPSHOT] For now, we are not cleaning up a key in
// active DB's deletedTable if any one of the keys in
// RepeatedOmKeyInfo exists in last snapshot's
key/fileTable.
// Might need to refactor OMKeyDeleteRequest first to take
// actual reclaimed key objectIDs as input
// in order to avoid any race condition.
blockGroupList.clear();
break;
}
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1395,16 +1395,35 @@ private PersistedUserVolumeInfo getVolumesByUser(String
userNameKey)
}
}
- @Override
- public List<BlockGroup> getPendingDeletionKeys(final int keyCount)
- throws IOException {
+ /**
+ * Returns a list of pending deletion key info that ups to the given count.
+ * Each entry is a {@link BlockGroup}, which contains the info about the key
+ * name and all its associated block IDs. A pending deletion key is stored
+ * with #deleting# prefix in OM DB.
Review Comment:
Is this `#deleting#` prefix already there in active DB's `deletedTable` for
those keys?
--
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]
