You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/10/28 08:30:40 UTC
[GitHub] [ozone] sadanand48 opened a new pull request, #3906: HDDS-5866. Discrepancy in Trash directory in ofs vs o3fs.
sadanand48 opened a new pull request, #3906:
URL: https://github.com/apache/ozone/pull/3906
## What changes were proposed in this pull request?
When moving a file/dir to trash, the destination path is different for ofs and o3fs. The change here is to make it same for both.
```
o3fs -> /<vol>/<buck>/.Trash/<user>/Current/..<dir if any>..
ofs -> /<vol>/<buck>/.Trash/<user>/Current/<vol>/<buck>/..<dir if any>..
```
Omitting volume and bucket for ofs key since its redundant info as for a particular ozone key already volume and bucket is known.
Most of the code for moveToTrash is duplicated from the [hadoop client code](https://github.com/apache/hadoop/blob/616cea2e8068e990d24057d2b0d6090f35e21371/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/TrashPolicyDefault.java#L123 ) and only the below if condition is a modification to the method.
If this part is not duplicated and the path is trimmed of volume and bucket name and passed to `super.moveToTrash(path)` it will fail as part of getFileStatus call as it would call `getFileStatus(ofs://serviceId/key)`.
```java
if (fs.getUri().getScheme().equals(OzoneConsts.OZONE_OFS_URI_SCHEME)) {
OFSPath ofsPath = new OFSPath(path);
// trimming volume and bucket in order to be compatible with o3fs
// Also including volume and bucket name in the path is redundant as
// the key is already in a particular volume and bucket.
Path trimmedVolumeAndBucket =
new Path(OzoneConsts.OZONE_URI_DELIMITER
+ ofsPath.getKeyName());
trashPath = makeTrashRelativePath(trashCurrent, trimmedVolumeAndBucket);
baseTrashPath = makeTrashRelativePath(trashCurrent,
trimmedVolumeAndBucket.getParent());
} else {
trashPath = makeTrashRelativePath(trashCurrent, path);
baseTrashPath = makeTrashRelativePath(trashCurrent, path.getParent());
}
```
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5866
## How was this patch tested?
unit 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] sadanand48 commented on a diff in pull request #3906: HDDS-5866. Discrepancy in Trash directory in ofs vs o3fs.
Posted by GitBox <gi...@apache.org>.
sadanand48 commented on code in PR #3906:
URL: https://github.com/apache/ozone/pull/3906#discussion_r1007802502
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java:
##########
@@ -1523,39 +1521,6 @@ public void testGetTrashRoots() throws IOException {
Assert.assertTrue(volume1.setOwner(prevOwner));
}
- /**
- * Check that files are moved to trash since it is enabled by
- * fs.rename(src, dst, options).
- */
- @Test
- @Flaky({"HDDS-5819", "HDDS-6451"})
Review Comment:
This test is flaky as it checks for existence of key in /Current but the checkpoint interval is in milliseconds so it might already be in checkpoint directory. This part is already tested in testTrash , hence removing this 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] neils-dev commented on a diff in pull request #3906: HDDS-5866. Discrepancy in Trash directory in ofs vs o3fs.
Posted by GitBox <gi...@apache.org>.
neils-dev commented on code in PR #3906:
URL: https://github.com/apache/ozone/pull/3906#discussion_r1009997419
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java:
##########
@@ -1523,39 +1521,6 @@ public void testGetTrashRoots() throws IOException {
Assert.assertTrue(volume1.setOwner(prevOwner));
}
- /**
- * Check that files are moved to trash since it is enabled by
- * fs.rename(src, dst, options).
- */
- @Test
- @Flaky({"HDDS-5819", "HDDS-6451"})
- public void testRenameToTrashEnabled() throws IOException {
Review Comment:
Thanks @sadanand48 for fixing the discrepancy between .Trash paths for ofs and o3fs with this patch. Overall, your changes look good with restructuring the `super.moveToTrash` (`hadoop TrashPolicyDefault`) call within the `TrashPolicyOzone` to make the trash paths the same.
Just one thing with removed flaky test. Can you add a check into the `testTrash` test to check for the existence of the key renamed to the `userTrashCurrent` **_prior_** to checking for purge? Currently the test just checks that the key is purged and not in that path - https://github.com/apache/ozone/blob/5a69806aeadd44ab520e38c32240034728d55479/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java#L1620
--
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] DaveTeng0 commented on a diff in pull request #3906: HDDS-5866. Discrepancy in Trash directory in ofs vs o3fs.
Posted by GitBox <gi...@apache.org>.
DaveTeng0 commented on code in PR #3906:
URL: https://github.com/apache/ozone/pull/3906#discussion_r1013308337
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java:
##########
@@ -1523,39 +1521,6 @@ public void testGetTrashRoots() throws IOException {
Assert.assertTrue(volume1.setOwner(prevOwner));
}
- /**
- * Check that files are moved to trash since it is enabled by
- * fs.rename(src, dst, options).
- */
- @Test
- @Flaky({"HDDS-5819", "HDDS-6451"})
Review Comment:
thanks for the info!
--
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] sadanand48 commented on a diff in pull request #3906: HDDS-5866. Discrepancy in Trash directory in ofs vs o3fs.
Posted by GitBox <gi...@apache.org>.
sadanand48 commented on code in PR #3906:
URL: https://github.com/apache/ozone/pull/3906#discussion_r1010866719
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java:
##########
@@ -1523,39 +1521,6 @@ public void testGetTrashRoots() throws IOException {
Assert.assertTrue(volume1.setOwner(prevOwner));
}
- /**
- * Check that files are moved to trash since it is enabled by
- * fs.rename(src, dst, options).
- */
- @Test
- @Flaky({"HDDS-5819", "HDDS-6451"})
- public void testRenameToTrashEnabled() throws IOException {
Review Comment:
Thanks @neils-dev for the review, added this check now.
--
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] neils-dev merged pull request #3906: HDDS-5866. Discrepancy in Trash directory in ofs vs o3fs.
Posted by GitBox <gi...@apache.org>.
neils-dev merged PR #3906:
URL: https://github.com/apache/ozone/pull/3906
--
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] neils-dev commented on pull request #3906: HDDS-5866. Discrepancy in Trash directory in ofs vs o3fs.
Posted by GitBox <gi...@apache.org>.
neils-dev commented on PR #3906:
URL: https://github.com/apache/ozone/pull/3906#issuecomment-1299409691
+1
Thanks @sadanand48, re-run the workflow for the integration tests; looks like an unrelated error.
--
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] sadanand48 commented on a diff in pull request #3906: HDDS-5866. Discrepancy in Trash directory in ofs vs o3fs.
Posted by GitBox <gi...@apache.org>.
sadanand48 commented on code in PR #3906:
URL: https://github.com/apache/ozone/pull/3906#discussion_r1007802502
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java:
##########
@@ -1523,39 +1521,6 @@ public void testGetTrashRoots() throws IOException {
Assert.assertTrue(volume1.setOwner(prevOwner));
}
- /**
- * Check that files are moved to trash since it is enabled by
- * fs.rename(src, dst, options).
- */
- @Test
- @Flaky({"HDDS-5819", "HDDS-6451"})
Review Comment:
This test is flaky as it checks for existence of key in /Current but the checkpoint interval is 1.5 seconds so it might already be in checkpoint directory. This part is already tested in testTrash , hence removing this 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