You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "aswinshakil (via GitHub)" <gi...@apache.org> on 2023/05/31 21:27:32 UTC
[GitHub] [ozone] aswinshakil opened a new pull request, #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
aswinshakil opened a new pull request, #4811:
URL: https://github.com/apache/ozone/pull/4811
## What changes were proposed in this pull request?
When a key is overwritten, it will still have the same object ID as the previous version of the key. So only the object ID check between snapshots is not enough to prove the key is same. This patch does an additional check on the `OMBlockLocationInfoGroup`.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-8686
## How was this patch tested?
The patch was tested using updated existing test.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4811:
URL: https://github.com/apache/ozone/pull/4811#discussion_r1220615101
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockLocationInfo.java:
##########
@@ -176,6 +176,20 @@ public String toString() {
+ '}';
}
+ public boolean equalsForSDS(Object o) {
Review Comment:
better name?
```suggestion
public boolean hasSameBlockAs(Object o) {
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a diff in pull request #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #4811:
URL: https://github.com/apache/ozone/pull/4811#discussion_r1232491692
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1571,11 +1560,33 @@ public List<BlockGroup> getPendingDeletionKeys(final int keyCount,
}
currentCount++;
}
- keyBlocksList.addAll(blockGroupList);
+
+ List<OmKeyInfo> notReclaimableKeyInfoList =
+ notReclaimableKeyInfo.getOmKeyInfoList();
+
+ // If all the versions are not reclaimable, then do nothing.
+ if (notReclaimableKeyInfoList.size() > 0 &&
+ notReclaimableKeyInfoList.size() !=
+ infoList.getOmKeyInfoList().size()) {
+ keysToModify.put(kv.getKey(), notReclaimableKeyInfo);
+ }
+
+ if (notReclaimableKeyInfoList.size() !=
+ infoList.getOmKeyInfoList().size()) {
+ keyBlocksList.addAll(blockGroupList);
+ }
}
}
}
- return keyBlocksList;
+ return new PendingKeysDeletion(keyBlocksList, keysToModify);
+ }
+
+ private boolean versionExistsInPreviousSnapshot(OmKeyInfo omKeyInfo,
+ OmKeyInfo info, RepeatedOmKeyInfo delOmKeyInfo) {
+ return (omKeyInfo != null &&
Review Comment:
Thanks for the explanation. I will update the PR to have a hsync check.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4811:
URL: https://github.com/apache/ozone/pull/4811#discussion_r1225214505
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestSnapshotDeletingService.java:
##########
@@ -342,17 +384,55 @@ public void testSnapshotWithFSO() throws Exception {
assertTableRowCount(snapshotInfoTable, 1);
assertTableRowCount(renamedTable, 4);
assertTableRowCount(deletedDirTable, 3);
- assertTableRowCount(deletedTable, 10);
+
+ OmSnapshot snap1 = (OmSnapshot) om.getOmSnapshotManager()
+ .checkForSnapshot(VOLUME_NAME, BUCKET_NAME_TWO,
+ getSnapshotPrefix("snap1"), true);
+ Table<String, OmKeyInfo> snap1KeyTable =
+ snap1.getMetadataManager().getFileTable();
+ try (TableIterator<String, ? extends Table.KeyValue<String,
+ RepeatedOmKeyInfo>> iterator = deletedTable.iterator()) {
+ while (iterator.hasNext()) {
+ Table.KeyValue<String, RepeatedOmKeyInfo> next = iterator.next();
+ String activeDBDeletedKey = next.getKey();
+ if (activeDBDeletedKey.matches(".*/key1.*")) {
+ RepeatedOmKeyInfo activeDBDeleted = next.getValue();
+ OMMetadataManager metadataManager =
+ cluster.getOzoneManager().getMetadataManager();
+ GenericTestUtils.waitFor(() -> activeDBDeleted.getOmKeyInfoList()
+ .size() == 1, 100, 2000);
Review Comment:
I don't think this line would work as intended?
IIUC after `next.getValue()` is called above, the value is already deserialized into the local variable `activeDBDeleted`. And the local variable won't change even if the actual value in DB has been changed.
In the worst case it would just be wasting 2 seconds waiting for a variable that isn't changed in this case.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/PendingKeysDeletion.java:
##########
Review Comment:
Missing license header
https://github.com/apache/ozone/actions/runs/5249532132/jobs/9482499170#step:6:839
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotMoveDeletedKeysResponse.java:
##########
@@ -187,5 +187,35 @@ private RepeatedOmKeyInfo createRepeatedOmKeyInfo(List<KeyInfo> keyInfoList)
return result;
}
+
+ private RepeatedOmKeyInfo createRepeatedOmKeyInfo(
+ SnapshotMoveKeyInfos snapshotMoveKeyInfos,
+ OMMetadataManager metadataManager) throws IOException {
+ String dbKey = snapshotMoveKeyInfos.getKey();
+ List<KeyInfo> keyInfoList = snapshotMoveKeyInfos.getKeyInfosList();
+ // When older version of keys are moved to the next snapshot's deletedTable
+ // The newer version might also be in the next snapshot's deletedTable and
+ // it might overwrite. This is to avoid that and also avoid having
+ // orphans blocks.
+ RepeatedOmKeyInfo result = metadataManager.getDeletedTable().get(dbKey);
+
+ for (KeyInfo keyInfo : keyInfoList) {
+ OmKeyInfo omKeyInfo = OmKeyInfo.getFromProtobuf(keyInfo);
+ if (result == null) {
+ result = new RepeatedOmKeyInfo(omKeyInfo);
+ } else if (!isSameAsLatestOmKeyInfo(omKeyInfo, result)) {
+ result.addOmKeyInfo(omKeyInfo);
+ }
+ }
+
+ return result;
+ }
+
+ private boolean isSameAsLatestOmKeyInfo(OmKeyInfo omKeyInfo,
+ RepeatedOmKeyInfo result) {
+ int size = result.getOmKeyInfoList().size();
+ OmKeyInfo keyInfoFromRepeated = result.getOmKeyInfoList().get(size - 1);
Review Comment:
Assert `size > 0` ?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a diff in pull request #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #4811:
URL: https://github.com/apache/ozone/pull/4811#discussion_r1230124250
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1571,11 +1560,33 @@ public List<BlockGroup> getPendingDeletionKeys(final int keyCount,
}
currentCount++;
}
- keyBlocksList.addAll(blockGroupList);
+
+ List<OmKeyInfo> notReclaimableKeyInfoList =
+ notReclaimableKeyInfo.getOmKeyInfoList();
+
+ // If all the versions are not reclaimable, then do nothing.
+ if (notReclaimableKeyInfoList.size() > 0 &&
Review Comment:
For case 2, if all versions are reclaimable then notReclaimableKeyInfoList will be 0. So it will be added to `keyBlocksList` and it will be deleted by the PurgeRequest
```
if (notReclaimableKeyInfoList.size() !=
infoList.getOmKeyInfoList().size()) {
keyBlocksList.addAll(blockGroupList);
}
```
3) I don't see a case where we only delete a few blocks in `OMKeyInfo`. Let me know if there is some case.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on pull request #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on PR #4811:
URL: https://github.com/apache/ozone/pull/4811#issuecomment-1595507055
Thanks @smengcl @swamirishi @sumitagrawl for the review.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on pull request #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on PR #4811:
URL: https://github.com/apache/ozone/pull/4811#issuecomment-1581211261
@sumitagrawl When we overwrite a key, the object ID remains the same but the key is different.
Although at a time only one version exists (by default bucket version is turned off). During snapshot deletion, multiple versions of the keys with the same objectID are moved to either next snapshot or active db's `deletedTable`.
In this case, we might need to check the block locations as well to confirm if it is the same version of the key.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a diff in pull request #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #4811:
URL: https://github.com/apache/ozone/pull/4811#discussion_r1232891688
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyPurgeResponse.java:
##########
@@ -69,6 +77,22 @@ public void addToDBBatch(OMMetadataManager omMetadataManager,
}
} else {
processKeys(batchOperation, omMetadataManager);
+ processKeysToUpdate(batchOperation, omMetadataManager);
+ }
+ }
+
+ private void processKeysToUpdate(BatchOperation batchOp,
+ OMMetadataManager metadataManager) throws IOException {
+ if (keysToUpdateList == null) {
+ return;
+ }
+
+ for (SnapshotMoveKeyInfos keyToUpdate : keysToUpdateList) {
+ List<KeyInfo> keyInfosList = keyToUpdate.getKeyInfosList();
+ RepeatedOmKeyInfo repeatedOmKeyInfo =
+ createRepeatedOmKeyInfo(keyInfosList);
+ metadataManager.getDeletedTable().putWithBatch(batchOp,
Review Comment:
If all versions are reclaimable they are never put it `keysToModify`, they will be deleted by the `KeyDeletingService` itself. It won't even come here.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swamirishi commented on a diff in pull request #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4811:
URL: https://github.com/apache/ozone/pull/4811#discussion_r1227361334
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -610,6 +615,37 @@ public void submitRequest(OMRequest omRequest) {
}
}
+ public static boolean isBlockLocationInfoSame(OmKeyInfo prevKeyInfo,
+ OmKeyInfo deletedKeyInfo) {
+ OmKeyLocationInfoGroup deletedOmKeyLocation =
+ deletedKeyInfo.getLatestVersionLocations();
Review Comment:
I guess this is the blockLocationList correct me if I wrong, I wanted to just put another check on the number of versions are also same. Just to be on the safe side so that we don't end up deleting some random data because of some bug in the version maintenance. Say if they start supporting bucket versioning deletion of particular version.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a diff in pull request #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #4811:
URL: https://github.com/apache/ozone/pull/4811#discussion_r1231320385
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1571,11 +1560,33 @@ public List<BlockGroup> getPendingDeletionKeys(final int keyCount,
}
currentCount++;
}
- keyBlocksList.addAll(blockGroupList);
+
+ List<OmKeyInfo> notReclaimableKeyInfoList =
+ notReclaimableKeyInfo.getOmKeyInfoList();
+
+ // If all the versions are not reclaimable, then do nothing.
+ if (notReclaimableKeyInfoList.size() > 0 &&
+ notReclaimableKeyInfoList.size() !=
+ infoList.getOmKeyInfoList().size()) {
+ keysToModify.put(kv.getKey(), notReclaimableKeyInfo);
+ }
+
+ if (notReclaimableKeyInfoList.size() !=
+ infoList.getOmKeyInfoList().size()) {
+ keyBlocksList.addAll(blockGroupList);
+ }
}
}
}
- return keyBlocksList;
+ return new PendingKeysDeletion(keyBlocksList, keysToModify);
+ }
+
+ private boolean versionExistsInPreviousSnapshot(OmKeyInfo omKeyInfo,
+ OmKeyInfo info, RepeatedOmKeyInfo delOmKeyInfo) {
+ return (omKeyInfo != null &&
Review Comment:
I'm confused, In hsync the keys will be `OpenKeyTable` until it is committed right? We here only care about those keys in `keyTable` and `deletedTable`. Please correct me if I'm wrong.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a diff in pull request #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #4811:
URL: https://github.com/apache/ozone/pull/4811#discussion_r1227319765
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -610,6 +615,37 @@ public void submitRequest(OMRequest omRequest) {
}
}
+ public static boolean isBlockLocationInfoSame(OmKeyInfo prevKeyInfo,
+ OmKeyInfo deletedKeyInfo) {
+ OmKeyLocationInfoGroup deletedOmKeyLocation =
+ deletedKeyInfo.getLatestVersionLocations();
Review Comment:
There is already a check for the size of the locations list below.
```
if (deletedLocationList.size() != prevLocationList.size()) {
return false;
}
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on pull request #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on PR #4811:
URL: https://github.com/apache/ozone/pull/4811#issuecomment-1579944828
@aswinshakil I am not getting when object Id is same but content is different ? I can see only case is HSync.
So for Hsync, we can just check metadata if it belongs to Hsync or not, if Belongs, then there is a possibility of object can be different.
For other cases, it does not need to compare details of object.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on pull request #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on PR #4811:
URL: https://github.com/apache/ozone/pull/4811#issuecomment-1581886647
@aswinshakil Thanks for the info, overwrite case can have same objectId but different version of content.
IMO,
- Need to have unique key for file key in deletedTable (similar to uncommited block handling in commit request), this will ensure no block leak due to overwrite of key when conflict.
- keyDeletingService need compare objectId (as existing) and one of blockId if matching
- SnapshotDeletingService also need have similar comparison with additional blockID if matching
- Snapdiff also need have similar comparison for identify if object is same or different
I think, we can have a change in KeyInfo have have additional field - version, which can be auto-incremented in case of overwrite to distinguish different object and also for debugging identifying overwrite. This can be used to replace blockId compare.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swamirishi commented on a diff in pull request #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4811:
URL: https://github.com/apache/ozone/pull/4811#discussion_r1227361334
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -610,6 +615,37 @@ public void submitRequest(OMRequest omRequest) {
}
}
+ public static boolean isBlockLocationInfoSame(OmKeyInfo prevKeyInfo,
+ OmKeyInfo deletedKeyInfo) {
+ OmKeyLocationInfoGroup deletedOmKeyLocation =
+ deletedKeyInfo.getLatestVersionLocations();
Review Comment:
I guess this is the blockLocationList correct me if I wrong, I wanted to just put another check on the number of versions are also same. Just to be on the safe side so that we don't end up deleting some random data because of some bug in the version maintenance. Say if they start supporting bucket versioning deletion of particular version. Out here we are just checking it for the latestVersion
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil merged pull request #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil merged PR #4811:
URL: https://github.com/apache/ozone/pull/4811
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swamirishi commented on a diff in pull request #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #4811:
URL: https://github.com/apache/ozone/pull/4811#discussion_r1226573394
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManager.java:
##########
@@ -121,16 +123,18 @@ List<RepeatedOmKeyInfo> listTrash(String volumeName, String bucketName,
String startKeyName, String keyPrefix, int maxKeys) 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.
+ * Returns a Pair. First is 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. Second is a
+ * Mapping of Key-Value pair which is updated in the deletedTable.
*
* @param count max number of keys to return.
- * @return a list of {@link BlockGroup} representing keys and blocks.
+ * @return a Pair of list of {@link BlockGroup} representing keys and blocks,
+ * and a hashmap for key-value pair to be updated in the deletedTable.
* @throws IOException
*/
- List<BlockGroup> getPendingDeletionKeys(int count) throws IOException;
+ Pair<List<BlockGroup>, HashMap<String, RepeatedOmKeyInfo>>
Review Comment:
I guess we can move this into another class instead of using a pair & change HashMap to Map
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1571,11 +1562,33 @@ public List<BlockGroup> getPendingDeletionKeys(final int keyCount,
}
currentCount++;
}
- keyBlocksList.addAll(blockGroupList);
+
+ List<OmKeyInfo> notReclaimableKeyInfoList =
+ notReclaimableKeyInfo.getOmKeyInfoList();
+
+ // If all the versions are not reclaimable, then do nothing.
+ if (notReclaimableKeyInfoList.size() > 0 &&
+ notReclaimableKeyInfoList.size() !=
+ infoList.getOmKeyInfoList().size()) {
+ keysToModify.put(kv.getKey(), notReclaimableKeyInfo);
+ }
+
+ if (notReclaimableKeyInfoList.size() !=
+ infoList.getOmKeyInfoList().size()) {
+ keyBlocksList.addAll(blockGroupList);
+ }
}
}
}
- return keyBlocksList;
+ return Pair.of(keyBlocksList, keysToModify);
+ }
+
+ private boolean versionExistsInPreviousSnapshot(OmKeyInfo omKeyInfo,
Review Comment:
Parameter names nomenclature could be better.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -610,6 +615,37 @@ public void submitRequest(OMRequest omRequest) {
}
}
+ public static boolean isBlockLocationInfoSame(OmKeyInfo prevKeyInfo,
+ OmKeyInfo deletedKeyInfo) {
+ OmKeyLocationInfoGroup deletedOmKeyLocation =
+ deletedKeyInfo.getLatestVersionLocations();
Review Comment:
It would be safe to put a check on the size of the locations list.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a diff in pull request #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #4811:
URL: https://github.com/apache/ozone/pull/4811#discussion_r1228559830
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/PendingKeysDeletion.java:
##########
Review Comment:
Ah forgot it. Will add.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #4811:
URL: https://github.com/apache/ozone/pull/4811#discussion_r1232085258
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1571,11 +1560,33 @@ public List<BlockGroup> getPendingDeletionKeys(final int keyCount,
}
currentCount++;
}
- keyBlocksList.addAll(blockGroupList);
+
+ List<OmKeyInfo> notReclaimableKeyInfoList =
+ notReclaimableKeyInfo.getOmKeyInfoList();
+
+ // If all the versions are not reclaimable, then do nothing.
+ if (notReclaimableKeyInfoList.size() > 0 &&
+ notReclaimableKeyInfoList.size() !=
+ infoList.getOmKeyInfoList().size()) {
+ keysToModify.put(kv.getKey(), notReclaimableKeyInfo);
+ }
+
+ if (notReclaimableKeyInfoList.size() !=
+ infoList.getOmKeyInfoList().size()) {
+ keyBlocksList.addAll(blockGroupList);
+ }
}
}
}
- return keyBlocksList;
+ return new PendingKeysDeletion(keyBlocksList, keysToModify);
+ }
+
+ private boolean versionExistsInPreviousSnapshot(OmKeyInfo omKeyInfo,
+ OmKeyInfo info, RepeatedOmKeyInfo delOmKeyInfo) {
+ return (omKeyInfo != null &&
Review Comment:
@aswinshakil
In Hsync,
- Key will be present in openKeyTable and keyTable both
- when commit happens, the blocks at that moment will be flushed to keyTable, so that is available for read
- then further new blocks can be added in openKeyTable and flush to commit table
So there is a possibility,
- snapshot can have "key" in keyTable which is having partial data
- So both key needs to be concluded as same key only (no need compare blocks)
One way to solve is, previous key need to check in metada, if Hsync flag is present or not. If present, then no need compare blocks.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a diff in pull request #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #4811:
URL: https://github.com/apache/ozone/pull/4811#discussion_r1230104763
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1543,17 +1535,14 @@ public List<BlockGroup> getPendingDeletionKeys(final int keyCount,
// have to check deletedTable of previous snapshot
RepeatedOmKeyInfo delOmKeyInfo =
prevDeletedTable.get(prevDelTableDBKey);
- if ((omKeyInfo != null &&
- info.getObjectID() == omKeyInfo.getObjectID()) ||
- delOmKeyInfo != null) {
- // 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;
+ if (versionExistsInPreviousSnapshot(omKeyInfo,
+ info, delOmKeyInfo)) {
+ // If the infoList size is 1, there is nothing to split.
+ // We either delete it or skip it.
+ if (!(infoList.getOmKeyInfoList().size() == 1)) {
Review Comment:
It is being added to `notReclaimableKeyInfo`,
```
if (!(infoList.getOmKeyInfoList().size() == 1))
```
This here is checking for the case where size != 1, which also means size > 1.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a diff in pull request #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #4811:
URL: https://github.com/apache/ozone/pull/4811#discussion_r1228574813
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestSnapshotDeletingService.java:
##########
@@ -342,17 +384,55 @@ public void testSnapshotWithFSO() throws Exception {
assertTableRowCount(snapshotInfoTable, 1);
assertTableRowCount(renamedTable, 4);
assertTableRowCount(deletedDirTable, 3);
- assertTableRowCount(deletedTable, 10);
+
+ OmSnapshot snap1 = (OmSnapshot) om.getOmSnapshotManager()
+ .checkForSnapshot(VOLUME_NAME, BUCKET_NAME_TWO,
+ getSnapshotPrefix("snap1"), true);
+ Table<String, OmKeyInfo> snap1KeyTable =
+ snap1.getMetadataManager().getFileTable();
+ try (TableIterator<String, ? extends Table.KeyValue<String,
+ RepeatedOmKeyInfo>> iterator = deletedTable.iterator()) {
+ while (iterator.hasNext()) {
+ Table.KeyValue<String, RepeatedOmKeyInfo> next = iterator.next();
+ String activeDBDeletedKey = next.getKey();
+ if (activeDBDeletedKey.matches(".*/key1.*")) {
+ RepeatedOmKeyInfo activeDBDeleted = next.getValue();
+ OMMetadataManager metadataManager =
+ cluster.getOzoneManager().getMetadataManager();
+ GenericTestUtils.waitFor(() -> activeDBDeleted.getOmKeyInfoList()
+ .size() == 1, 100, 2000);
Review Comment:
Nice catch, You are right. We can just have assert instead of this. Will update the PR.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a diff in pull request #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #4811:
URL: https://github.com/apache/ozone/pull/4811#discussion_r1228576635
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -610,6 +615,37 @@ public void submitRequest(OMRequest omRequest) {
}
}
+ public static boolean isBlockLocationInfoSame(OmKeyInfo prevKeyInfo,
+ OmKeyInfo deletedKeyInfo) {
+ OmKeyLocationInfoGroup deletedOmKeyLocation =
+ deletedKeyInfo.getLatestVersionLocations();
Review Comment:
Oh, I get it now. Agreed. Will update the patch with necessary changes.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #4811: HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #4811:
URL: https://github.com/apache/ozone/pull/4811#discussion_r1229899139
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1543,17 +1535,14 @@ public List<BlockGroup> getPendingDeletionKeys(final int keyCount,
// have to check deletedTable of previous snapshot
RepeatedOmKeyInfo delOmKeyInfo =
prevDeletedTable.get(prevDelTableDBKey);
- if ((omKeyInfo != null &&
- info.getObjectID() == omKeyInfo.getObjectID()) ||
- delOmKeyInfo != null) {
- // 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;
+ if (versionExistsInPreviousSnapshot(omKeyInfo,
+ info, delOmKeyInfo)) {
+ // If the infoList size is 1, there is nothing to split.
+ // We either delete it or skip it.
+ if (!(infoList.getOmKeyInfoList().size() == 1)) {
Review Comment:
If having OmKeyInfoList() size is > "1", its neither added to notReclaimableKeyInfo nor to blockGroupList, so how cleanup will happen?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -530,11 +532,14 @@ private boolean isKeyReclaimable(
OmKeyInfo prevKeyInfo = renamedKey != null ? previousKeyTable
.get(renamedKey) : previousKeyTable.get(dbKey);
- if (prevKeyInfo == null) {
+ if (prevKeyInfo == null ||
+ prevKeyInfo.getObjectID() != deletedKeyInfo.getObjectID()) {
return true;
}
- return prevKeyInfo.getObjectID() != deletedKeyInfo.getObjectID();
+ // For key overwrite the objectID will remain the same, In this
Review Comment:
For Hsync, blockLocations may not be same, but still same object, may need handle this scenario
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyPurgeResponse.java:
##########
@@ -69,6 +77,22 @@ public void addToDBBatch(OMMetadataManager omMetadataManager,
}
} else {
processKeys(batchOperation, omMetadataManager);
+ processKeysToUpdate(batchOperation, omMetadataManager);
+ }
+ }
+
+ private void processKeysToUpdate(BatchOperation batchOp,
+ OMMetadataManager metadataManager) throws IOException {
+ if (keysToUpdateList == null) {
+ return;
+ }
+
+ for (SnapshotMoveKeyInfos keyToUpdate : keysToUpdateList) {
+ List<KeyInfo> keyInfosList = keyToUpdate.getKeyInfosList();
+ RepeatedOmKeyInfo repeatedOmKeyInfo =
+ createRepeatedOmKeyInfo(keyInfosList);
+ metadataManager.getDeletedTable().putWithBatch(batchOp,
Review Comment:
If this is put back to same deletedTable, keyDeletingService keeps on retrying ... do the above case of all version re-claimable -- not added to any list used to break this retry ?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1571,11 +1560,33 @@ public List<BlockGroup> getPendingDeletionKeys(final int keyCount,
}
currentCount++;
}
- keyBlocksList.addAll(blockGroupList);
+
+ List<OmKeyInfo> notReclaimableKeyInfoList =
+ notReclaimableKeyInfo.getOmKeyInfoList();
+
+ // If all the versions are not reclaimable, then do nothing.
+ if (notReclaimableKeyInfoList.size() > 0 &&
Review Comment:
3 case, not getting if all scenario handled:
1. all versions are not reclaimable - handled
2. all versions are reclaimable - not getting how handled
3. blocks are added if few blocks are reclaimable
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1571,11 +1560,33 @@ public List<BlockGroup> getPendingDeletionKeys(final int keyCount,
}
currentCount++;
}
- keyBlocksList.addAll(blockGroupList);
+
+ List<OmKeyInfo> notReclaimableKeyInfoList =
+ notReclaimableKeyInfo.getOmKeyInfoList();
+
+ // If all the versions are not reclaimable, then do nothing.
+ if (notReclaimableKeyInfoList.size() > 0 &&
+ notReclaimableKeyInfoList.size() !=
+ infoList.getOmKeyInfoList().size()) {
+ keysToModify.put(kv.getKey(), notReclaimableKeyInfo);
+ }
+
+ if (notReclaimableKeyInfoList.size() !=
+ infoList.getOmKeyInfoList().size()) {
+ keyBlocksList.addAll(blockGroupList);
+ }
}
}
}
- return keyBlocksList;
+ return new PendingKeysDeletion(keyBlocksList, keysToModify);
+ }
+
+ private boolean versionExistsInPreviousSnapshot(OmKeyInfo omKeyInfo,
+ OmKeyInfo info, RepeatedOmKeyInfo delOmKeyInfo) {
+ return (omKeyInfo != null &&
Review Comment:
Need handle for HSync, where object will be same but blocks are different as append mechanism
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org