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/04/21 16:35:17 UTC
[GitHub] [ozone] aswinshakil opened a new pull request, #4607: HDDS-8435. [Snapshot] Handle Directory renames for FSO Buckets.
aswinshakil opened a new pull request, #4607:
URL: https://github.com/apache/ozone/pull/4607
## What changes were proposed in this pull request?
When directories are renamed, this information is not tracked. It is required by the`SnapshotDeletingService` to check the previous snapshot's `directoryTable` and check if the renamed key is present.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-8435
## How was this patch tested?
Existing tests.
--
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 #4607: HDDS-8435. [Snapshot] Handle Directory renames for FSO Buckets.
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on PR #4607:
URL: https://github.com/apache/ozone/pull/4607#issuecomment-1518123495
The `snapshotRenamedTable` only tracks the `tableKey` of the directory that was renamed and it is only used by `SnapshotDeletingService`. When a directory is renamed after a snapshot for a bucket is taken. The `SnapshotDeletingService` looks into the `directoryTable` of the previous snapshot, but when the directory is renamed the `tableKey` will be changed. The `snapshotRenamedTable` only acts as reference to check the renamed directories in between snapshot.
> For example, what happens if a file is being written under a directory that is renamed? It will file to commit. Has this been considered?
This shouldn't be an issue, Everything would go through the original flow.
--
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 #4607: HDDS-8435. [Snapshot] Handle Directory renames for FSO Buckets.
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #4607:
URL: https://github.com/apache/ozone/pull/4607#discussion_r1179476701
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/codec/OMDBDefinition.java:
##########
@@ -231,22 +231,22 @@ String.class, new StringCodec(), OmKeyInfo.class,
new OmDBSnapshotInfoCodec());
/**
- * SnapshotRenamedKeyTable that complements the keyTable (or fileTable)
- * entries of the immediately previous snapshot in the same snapshot
- * scope (bucket or volume).
+ * SnapshotRenamedTable that complements the keyTable (or fileTable)
+ * and dirTable entries of the immediately previous snapshot in the
+ * same snapshot scope (bucket or volume).
* <p>
- * Key renames between the two subsequent snapshots are captured, this
+ * Key/Dir renames between the two subsequent snapshots are captured, this
* information is used in {@link SnapshotDeletingService} to check if the
- * renamedKey is present in the previous snapshot's keyTable
+ * renamedKey or renamedDir is present in the previous snapshot's keyTable
* (or fileTable).
*/
public static final DBColumnFamilyDefinition<String, String>
SNAPSHOT_RENAMED_KEY_TABLE =
Review Comment:
Nice catch. Fixed it.
--
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 pull request #4607: HDDS-8435. [Snapshot] Handle Directory renames for FSO Buckets.
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #4607:
URL: https://github.com/apache/ozone/pull/4607#issuecomment-1532175600
Thanks @ashishkumar50 for the patch. Thanks @devmadhuu and @ashishkumar50 for the reviews.
--
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 #4607: HDDS-8435. [Snapshot] Handle Directory renames for FSO Buckets.
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4607:
URL: https://github.com/apache/ozone/pull/4607#discussion_r1179725518
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyRenameResponseWithFSO.java:
##########
@@ -91,20 +91,19 @@ public void addToDBBatch(OMMetadataManager omMetadataManager,
.deleteWithBatch(batchOperation, getFromKeyName());
omMetadataManager.getKeyTable(getBucketLayout())
.putWithBatch(batchOperation, getToKeyName(), getRenameKeyInfo());
+ }
Review Comment:
got it. makes sense. thx Aswin
--
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 merged pull request #4607: HDDS-8435. [Snapshot] Handle Directory renames for FSO Buckets.
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl merged PR #4607:
URL: https://github.com/apache/ozone/pull/4607
--
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 #4607: HDDS-8435. [Snapshot] Handle Directory renames for FSO Buckets.
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on PR #4607:
URL: https://github.com/apache/ozone/pull/4607#issuecomment-1526065009
Thanks @smengcl for the review. I have updated the PR, please take a look at it.
--
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] devmadhuu commented on a diff in pull request #4607: HDDS-8435. [Snapshot] Handle Directory renames for FSO Buckets.
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #4607:
URL: https://github.com/apache/ozone/pull/4607#discussion_r1174884166
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotCreateRequest.java:
##########
@@ -256,7 +256,7 @@ public void testEmptySnapshotRenamedKeyTable() throws Exception {
renameKey("key1", "key2");
Review Comment:
@aswinshakil can we add an additional test case for renaming the directory and verify.
--
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] jojochuang commented on pull request #4607: HDDS-8435. [Snapshot] Handle Directory renames for FSO Buckets.
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #4607:
URL: https://github.com/apache/ozone/pull/4607#issuecomment-1518106016
HI i haven't reviewed the PR because i don't know much about the snapshot design.
But I know for a fact directory rename could be problematic for FSO. For example, what happens if a file is being written under a directory that is renamed? It will file to commit. Has this been considered?
--
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 #4607: HDDS-8435. [Snapshot] Handle Directory renames for FSO Buckets.
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #4607:
URL: https://github.com/apache/ozone/pull/4607#discussion_r1179475470
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyRenameResponseWithFSO.java:
##########
@@ -91,20 +91,19 @@ public void addToDBBatch(OMMetadataManager omMetadataManager,
.deleteWithBatch(batchOperation, getFromKeyName());
omMetadataManager.getKeyTable(getBucketLayout())
.putWithBatch(batchOperation, getToKeyName(), getRenameKeyInfo());
+ }
Review Comment:
I don't think this is a bug. Previously we only kept snapshot checks in the `else` part as we only tracked keys. Now I moved it outside as we track both keys and directory with this 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 #4607: HDDS-8435. [Snapshot] Handle Directory renames for FSO Buckets.
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #4607:
URL: https://github.com/apache/ozone/pull/4607#discussion_r1175712644
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotCreateRequest.java:
##########
@@ -256,7 +256,7 @@ public void testEmptySnapshotRenamedKeyTable() throws Exception {
renameKey("key1", "key2");
Review Comment:
Done. Added it.
--
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 #4607: HDDS-8435. [Snapshot] Handle Directory renames for FSO Buckets.
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4607:
URL: https://github.com/apache/ozone/pull/4607#discussion_r1178493929
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -209,12 +209,12 @@ public BackgroundTaskResult call() throws Exception {
// 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<HddsProtos.KeyValue> renamedList = new ArrayList<>();
Review Comment:
```suggestion
// A list of renamed keys/files/dirs
List<HddsProtos.KeyValue> renamedList = new ArrayList<>();
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -196,7 +197,10 @@ public class OmMetadataManagerImpl implements OMMetadataManager,
* |-------------------------------------------------------------------------|
* | snapshotInfoTable | /volume/bucket/snapshotName -> SnapshotInfo |
* |-------------------------------------------------------------------------|
- * |snapshotRenamedKeyTable| /volumeName/bucketName/objectID -> /v/b/origKey |
+ * | snapshotRenamedTable | /volumeName/bucketName/objectID -> |
+ * | | /volumeId/bucketId/parentId/dirName |
+ * | | /volumeId/bucketId/parentId/fileName |
+ * | | /volumeName/bucketName/keyName |
Review Comment:
further clarification
```suggestion
* | snapshotRenamedTable | /volumeName/bucketName/objectID -> One of: |
* | | 1. /volumeId/bucketId/parentId/dirName |
* | | 2. /volumeId/bucketId/parentId/fileName |
* | | 3. /volumeName/bucketName/keyName |
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyRenameResponseWithFSO.java:
##########
@@ -91,20 +91,19 @@ public void addToDBBatch(OMMetadataManager omMetadataManager,
.deleteWithBatch(batchOperation, getFromKeyName());
omMetadataManager.getKeyTable(getBucketLayout())
.putWithBatch(batchOperation, getToKeyName(), getRenameKeyInfo());
+ }
Review Comment:
good catch
can we add a line in the PR description to say this bug is fixed?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/codec/OMDBDefinition.java:
##########
@@ -231,22 +231,22 @@ String.class, new StringCodec(), OmKeyInfo.class,
new OmDBSnapshotInfoCodec());
/**
- * SnapshotRenamedKeyTable that complements the keyTable (or fileTable)
- * entries of the immediately previous snapshot in the same snapshot
- * scope (bucket or volume).
+ * SnapshotRenamedTable that complements the keyTable (or fileTable)
+ * and dirTable entries of the immediately previous snapshot in the
+ * same snapshot scope (bucket or volume).
* <p>
- * Key renames between the two subsequent snapshots are captured, this
+ * Key/Dir renames between the two subsequent snapshots are captured, this
* information is used in {@link SnapshotDeletingService} to check if the
- * renamedKey is present in the previous snapshot's keyTable
+ * renamedKey or renamedDir is present in the previous snapshot's keyTable
* (or fileTable).
*/
public static final DBColumnFamilyDefinition<String, String>
SNAPSHOT_RENAMED_KEY_TABLE =
Review Comment:
Rename this var as well?
```suggestion
public static final DBColumnFamilyDefinition<String, String>
SNAPSHOT_RENAMED_TABLE =
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -449,18 +452,42 @@ private void submitSnapshotMoveDeletedKeys(SnapshotInfo snapInfo,
private boolean checkDirReclaimable(
Table.KeyValue<String, OmKeyInfo> deletedDir,
- Table<String, OmDirectoryInfo> previousDirTable) throws IOException {
+ Table<String, OmDirectoryInfo> previousDirTable,
+ Table<String, String> renamedTable,
+ List<HddsProtos.KeyValue> renamedList) throws IOException {
if (previousDirTable == null) {
return true;
}
String deletedDirDbKey = deletedDir.getKey();
OmKeyInfo deletedDirInfo = deletedDir.getValue();
- // In OMKeyDeleteResponseWithFSO OzonePathKey is converted to
- // OzoneDeletePathKey. Changing it back to check the previous DirTable.
- String prevDbKey = ozoneManager.getMetadataManager()
- .getOzoneDeletePathDirKey(deletedDirDbKey);
+ String dbRenameKey = ozoneManager.getMetadataManager().getRenameKey(
+ deletedDirInfo.getVolumeName(), deletedDirInfo.getBucketName(),
+ deletedDirInfo.getObjectID());
+
+ /*
+ snapshotRenamedTable: /volumeName/bucketName/objectID ->
+ /volumeId/bucketId/parentId/dirName
+ */
+ String renamedKey = renamedTable.getIfExist(dbRenameKey);
Review Comment:
```suggestion
String dbKeyBeforeRename = renamedTable.getIfExist(dbRenameKey);
```
--
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] ashishkumar50 commented on a diff in pull request #4607: HDDS-8435. [Snapshot] Handle Directory renames for FSO Buckets.
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on code in PR #4607:
URL: https://github.com/apache/ozone/pull/4607#discussion_r1180342504
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotCreateRequest.java:
##########
@@ -326,11 +334,42 @@ private void renameKey(String fromKey, String toKey) throws IOException {
new OMKeyRenameResponse(omResponse, fromKeyInfo.getKeyName(),
toKeyInfo.getKeyName(), toKeyInfo);
- Assert.assertTrue(omMetadataManager.getSnapshotRenamedKeyTable().isEmpty());
omKeyRenameResponse.addToDBBatch(omMetadataManager, batchOperation);
omMetadataManager.getStore().commitBatchOperation(batchOperation);
}
+ private void renameDir(String fromKey, String toKey, long offset)
+ throws Exception {
+ String fromKeyParentName = UUID.randomUUID().toString();
+ OmKeyInfo fromKeyParent = OMRequestTestUtils.createOmKeyInfo(volumeName,
+ bucketName, fromKeyParentName, HddsProtos.ReplicationType.RATIS,
+ HddsProtos.ReplicationFactor.THREE, 100L);
+
+ OmKeyInfo toKeyInfo = addKey(fromKey, offset + 4L);
Review Comment:
This need to be
OmKeyInfo toKeyInfo = addKey(**toKey**, offset + 4L);
OmKeyInfo fromKeyInfo = addKey(**fromKey**, offset + 5L); ?
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotCreateRequest.java:
##########
@@ -311,9 +318,10 @@ public void testEntryExists() throws Exception {
omResponse.getStatus());
}
- private void renameKey(String fromKey, String toKey) throws IOException {
- OmKeyInfo toKeyInfo = addKey(fromKey);
- OmKeyInfo fromKeyInfo = addKey(toKey);
+ private void renameKey(String fromKey, String toKey, long offset)
+ throws IOException {
+ OmKeyInfo toKeyInfo = addKey(fromKey, offset + 1L);
Review Comment:
This need to be
OmKeyInfo toKeyInfo = addKey(toKey, offset + 1L);
OmKeyInfo fromKeyInfo = addKey(fromKey, offset + 2L); ?
--
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 #4607: HDDS-8435. [Snapshot] Handle Directory renames for FSO Buckets.
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #4607:
URL: https://github.com/apache/ozone/pull/4607#discussion_r1180590374
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotCreateRequest.java:
##########
@@ -326,11 +334,42 @@ private void renameKey(String fromKey, String toKey) throws IOException {
new OMKeyRenameResponse(omResponse, fromKeyInfo.getKeyName(),
toKeyInfo.getKeyName(), toKeyInfo);
- Assert.assertTrue(omMetadataManager.getSnapshotRenamedKeyTable().isEmpty());
omKeyRenameResponse.addToDBBatch(omMetadataManager, batchOperation);
omMetadataManager.getStore().commitBatchOperation(batchOperation);
}
+ private void renameDir(String fromKey, String toKey, long offset)
+ throws Exception {
+ String fromKeyParentName = UUID.randomUUID().toString();
+ OmKeyInfo fromKeyParent = OMRequestTestUtils.createOmKeyInfo(volumeName,
+ bucketName, fromKeyParentName, HddsProtos.ReplicationType.RATIS,
+ HddsProtos.ReplicationFactor.THREE, 100L);
+
+ OmKeyInfo toKeyInfo = addKey(fromKey, offset + 4L);
Review Comment:
Thanks for pointing it out. Updated it.
--
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