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 2020/08/03 10:44:34 UTC
[GitHub] [hadoop-ozone] smengcl opened a new pull request #1286: HDDS-4040. OFS should use deleteObjects when deleting directory
smengcl opened a new pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286
## What changes were proposed in this pull request?
This Jira is to use deleteObjects in OFS delete now that HDDS-3286 is committed.
Currently when ozone.om.enable.filesystem.paths is enabled it normalizes the path, so using deleteKey for delete directory will fail.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4040
## How was this patch tested?
Untested yet.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on pull request #1286: HDDS-4040. [OFS] BasicRootedOzoneFileSystem to support batchDelete
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#issuecomment-672634366
Thanks for reviewing the patch @adoroszlai . I have made some changes accordingly. Please take another look. Thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1286: HDDS-4040. OFS should use deleteObjects when deleting directory
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#discussion_r464846912
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
##########
@@ -1038,4 +1043,41 @@ public void testRenameToTrashDisabled() throws IOException {
ofs.delete(trashRoot, true);
}
+ @Test
+ public void testFileDelete() throws Exception {
+ Path grandparent = new Path(bucketPath, "testBatchDelete");
+ Path parent = new Path(grandparent, "parent");
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1286: HDDS-4040. OFS should use deleteObjects when deleting directory
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#discussion_r467194687
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
##########
@@ -457,38 +458,79 @@ public boolean deleteObject(String path) {
}
}
+ /**
+ * Helper function to check if the list of key paths are in the same volume
+ * and same bucket.
+ */
+ private boolean areInSameBucket(List<String> keyNameList) {
+ if (keyNameList.size() == 0) {
+ return true;
+ }
+ String firstKeyPath = keyNameList.get(0);
+ final String volAndBucket = new OFSPath(firstKeyPath).getNonKeyPath();
+ // If any key path's volume and bucket from the second element and on
+ // in the list doesn't match the first element's, hasDifferentVolAndBucket
+ // would be true
+ boolean hasDifferentVolAndBucket = keyNameList.stream().skip(1)
+ .anyMatch(p -> !(new OFSPath(p).getNonKeyPath().equals(volAndBucket)));
+ return !hasDifferentVolAndBucket;
+ }
+
/**
* Helper method to delete an object specified by key name in bucket.
*
- * @param pathList key name list to be deleted
+ * Only supports deleting keys in the same bucket in one call.
+ *
+ * Each item in the given list should be the String of an OFS path:
+ * e.g. ofs://om/vol1/buck1/k1
+ *
+ * @param keyNameList key name list to be deleted
* @return true if the key is deleted, false otherwise
*/
@Override
- public boolean deleteObjects(List<String> pathList) {
- // TODO: we will support deleteObjects in ofs.
- LOG.error("ofs currently does not support deleteObjects");
- return false;
+ public boolean deleteObjects(List<String> keyNameList) {
+ LOG.trace("issuing delete for keys: {}", keyNameList);
+ if (keyNameList.size() == 0) {
+ return false;
+ }
+ // Sanity check. Support only deleting a list of keys in the same bucket
+ if (!areInSameBucket(keyNameList)) {
+ LOG.error("Deleting keys from different buckets in a single batch "
+ + "is not supported.");
+ return false;
+ }
+ try {
+ OFSPath firstKeyPath = new OFSPath(keyNameList.get(0));
+ OzoneBucket bucket = getBucket(firstKeyPath, false);
+ return deleteObjects(bucket, keyNameList);
+ } catch (IOException ioe) {
+ LOG.error("delete key failed: {}", ioe.getMessage());
+ return false;
+ }
}
/**
* Package-private helper function to reduce calls to getBucket().
+ *
+ * This will be faster than the public variant of the method since this
+ * doesn't verify the same-bucket condition.
+ *
* @param bucket Bucket to operate in.
- * @param path Path to delete.
- * @return true if operation succeeded, false upon IOException.
+ * @param keyNameList key name list to be deleted.
+ * @return true if operation succeeded, false on IOException.
*/
- boolean deleteObject(OzoneBucket bucket, String path) {
- LOG.trace("issuing delete for path to key: {}", path);
- incrementCounter(Statistic.OBJECTS_DELETED);
- OFSPath ofsPath = new OFSPath(path);
- String keyName = ofsPath.getKeyName();
- if (keyName.length() == 0) {
- return false;
- }
+ boolean deleteObjects(OzoneBucket bucket, List<String> keyNameList) {
+ LOG.trace("issuing delete in volume: {}, bucket: {} for keys: {}",
+ bucket.getVolumeName(), bucket.getName(), keyNameList);
+ List<String> keyList = keyNameList.stream()
+ .map(p -> new OFSPath(p).getKeyName())
+ .collect(Collectors.toList());
try {
- bucket.deleteKey(keyName);
+ incrementCounter(Statistic.OBJECTS_DELETED);
Review comment:
Yes you are right. I do notice this when making the patch. I was just trying to make it consistent with the o3fs code.
Should I make the change in both o3fs and ofs here in this PR? One point is I may need to add a new method to increment the stat by X rather than only 1 (for a more elegant solution).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1286: HDDS-4040. OFS should use deleteObjects when deleting directory
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#discussion_r464775619
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
##########
@@ -1038,4 +1043,41 @@ public void testRenameToTrashDisabled() throws IOException {
ofs.delete(trashRoot, true);
}
+ @Test
+ public void testFileDelete() throws Exception {
+ Path grandparent = new Path(bucketPath, "testBatchDelete");
+ Path parent = new Path(grandparent, "parent");
Review comment:
Make this test parameterized to test with "ozone.om.enable.filesystem.paths" enabled and disabled similar to TestOzoneFileSystem
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] adoroszlai commented on pull request #1286: HDDS-4040. [OFS] BasicRootedOzoneFileSystem to support batchDelete
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#issuecomment-672647292
Thanks @smengcl for updating the patch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on pull request #1286: HDDS-4040. OFS should use deleteObjects when deleting directory
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#issuecomment-669533265
>It seems adding parameterized test for TestRootedOzoneFileSystem tripped the time out.
Do we need to increase time out in 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] adoroszlai merged pull request #1286: HDDS-4040. [OFS] BasicRootedOzoneFileSystem to support batchDelete
Posted by GitBox <gi...@apache.org>.
adoroszlai merged pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1286: HDDS-4040. [OFS] BasicRootedOzoneFileSystem to support batchDelete
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#discussion_r468176686
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneFileSystem.java
##########
@@ -220,17 +220,22 @@ protected InputStream createFSInputStream(InputStream inputStream) {
return new OzoneFSInputStream(inputStream, statistics);
}
+ @Deprecated
protected void incrementCounter(Statistic statistic) {
//don't do anyting in this default implementation.
Review comment:
~~This method body is intentionally left empty in HDDS-1333 for compatibility fix I believe. Is this no longer the case?~~ should be fine since the change is only calling another empty method. but I don't see the point of doing this as we are overriding this in `OzoneFileSystem`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on pull request #1286: HDDS-4040. OFS should use deleteObjects when deleting directory
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#issuecomment-670091470
> > @bharatviswa504 Now that we can prove the tests work with both ozone.om.enable.filesystem.paths options. Should we >proceed with [fdbc71c](https://github.com/apache/hadoop-ozone/commit/fdbc71cdabcfa846d538878f80bb602a37a63528) ?
>
> Can we use @BeforeClass for starting MiniOzoneCluster instead for each test, that would save time.
> I would like to keep this parameterized, if anything is broken it would be easily caught, as default is false. (because of this, there were some cases missed)
Good idea. Just have to make everything static. Let me try.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on pull request #1286: HDDS-4040. OFS should use deleteObjects when deleting directory
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#issuecomment-670082854
I ran `TestRootedOzoneFileSystem` locally again. It seems the problem is it is taking significant longer (double the time spent) to run the test with the parameter (14m 52s total for both, about 7m 30s for each) than without the parameter (7m 47s). Good new is all tests pass.
Another observation. On the GH workflow run, without parameterization we were spending around 9.5min for `TestRootedOzoneFileSystem` without parameterization:
https://github.com/apache/hadoop-ozone/runs/941577903
```
[INFO] Running org.apache.hadoop.fs.ozone.TestOzoneFileSystem
[INFO] Tests run: 10, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 251.496 s - in org.apache.hadoop.fs.ozone.TestOzoneFileSystem
...
[INFO] Running org.apache.hadoop.fs.ozone.TestRootedOzoneFileSystem
[INFO] Tests run: 24, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 566.177 s - in org.apache.hadoop.fs.ozone.TestRootedOzoneFileSystem
...
[INFO] Running org.apache.hadoop.fs.ozone.TestOzoneFileInterfaces
[WARNING] Tests run: 18, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 381.674 s - in org.apache.hadoop.fs.ozone.TestOzoneFileInterfaces
```
With parameterization we can expect it to be spending over 1100s (18min) on this one test suite.. which is not good.
We will need to reduce the run time of the test suite. Probably should do this in another jira.
Also I haven't figured out where the timeout is imposed as of now. @adoroszlai Do you know where [this](https://github.com/apache/hadoop-ozone/runs/946974224) 45m-46m timeout is imposed?
@bharatviswa504 Now that we can prove the tests work with both `ozone.om.enable.filesystem.paths` options. Should we proceed with fdbc71cdabcfa846d538878f80bb602a37a63528 ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1286: HDDS-4040. [OFS] BasicRootedOzoneFileSystem to support batchDelete
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#discussion_r468181796
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
##########
@@ -457,45 +463,83 @@ public boolean deleteObject(String path) {
}
}
+ /**
+ * Helper function to check if the list of key paths are in the same volume
+ * and same bucket.
+ */
+ private boolean areInSameBucket(List<String> keyNameList) {
+ if (keyNameList.size() == 0) {
+ return true;
+ }
+ String firstKeyPath = keyNameList.get(0);
+ final String volAndBucket = new OFSPath(firstKeyPath).getNonKeyPath();
+ // If any key path's volume and bucket from the second element and on
+ // in the list doesn't match the first element's, hasDifferentVolAndBucket
+ // would be true
+ boolean hasDifferentVolAndBucket = keyNameList.stream().skip(1)
+ .anyMatch(p -> !(new OFSPath(p).getNonKeyPath().equals(volAndBucket)));
+ return !hasDifferentVolAndBucket;
Review comment:
Good catch!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1286: HDDS-4040. [OFS] BasicRootedOzoneFileSystem to support batchDelete
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#discussion_r468176686
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneFileSystem.java
##########
@@ -220,17 +220,22 @@ protected InputStream createFSInputStream(InputStream inputStream) {
return new OzoneFSInputStream(inputStream, statistics);
}
+ @Deprecated
protected void incrementCounter(Statistic statistic) {
//don't do anyting in this default implementation.
Review comment:
This method body is intentionally left empty in HDDS-1333 for compatibility fix I believe.
Is this no longer the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1286: HDDS-4040. [OFS] BasicRootedOzoneFileSystem to support batchDelete
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#discussion_r467211897
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
##########
@@ -457,38 +458,79 @@ public boolean deleteObject(String path) {
}
}
+ /**
+ * Helper function to check if the list of key paths are in the same volume
+ * and same bucket.
+ */
+ private boolean areInSameBucket(List<String> keyNameList) {
+ if (keyNameList.size() == 0) {
+ return true;
+ }
+ String firstKeyPath = keyNameList.get(0);
+ final String volAndBucket = new OFSPath(firstKeyPath).getNonKeyPath();
+ // If any key path's volume and bucket from the second element and on
+ // in the list doesn't match the first element's, hasDifferentVolAndBucket
+ // would be true
+ boolean hasDifferentVolAndBucket = keyNameList.stream().skip(1)
+ .anyMatch(p -> !(new OFSPath(p).getNonKeyPath().equals(volAndBucket)));
+ return !hasDifferentVolAndBucket;
+ }
+
/**
* Helper method to delete an object specified by key name in bucket.
*
- * @param pathList key name list to be deleted
+ * Only supports deleting keys in the same bucket in one call.
+ *
+ * Each item in the given list should be the String of an OFS path:
+ * e.g. ofs://om/vol1/buck1/k1
+ *
+ * @param keyNameList key name list to be deleted
* @return true if the key is deleted, false otherwise
*/
@Override
- public boolean deleteObjects(List<String> pathList) {
- // TODO: we will support deleteObjects in ofs.
- LOG.error("ofs currently does not support deleteObjects");
- return false;
+ public boolean deleteObjects(List<String> keyNameList) {
+ LOG.trace("issuing delete for keys: {}", keyNameList);
+ if (keyNameList.size() == 0) {
+ return false;
+ }
+ // Sanity check. Support only deleting a list of keys in the same bucket
+ if (!areInSameBucket(keyNameList)) {
+ LOG.error("Deleting keys from different buckets in a single batch "
+ + "is not supported.");
+ return false;
+ }
+ try {
+ OFSPath firstKeyPath = new OFSPath(keyNameList.get(0));
+ OzoneBucket bucket = getBucket(firstKeyPath, false);
+ return deleteObjects(bucket, keyNameList);
+ } catch (IOException ioe) {
+ LOG.error("delete key failed: {}", ioe.getMessage());
+ return false;
+ }
}
/**
* Package-private helper function to reduce calls to getBucket().
+ *
+ * This will be faster than the public variant of the method since this
+ * doesn't verify the same-bucket condition.
+ *
* @param bucket Bucket to operate in.
- * @param path Path to delete.
- * @return true if operation succeeded, false upon IOException.
+ * @param keyNameList key name list to be deleted.
+ * @return true if operation succeeded, false on IOException.
*/
- boolean deleteObject(OzoneBucket bucket, String path) {
- LOG.trace("issuing delete for path to key: {}", path);
- incrementCounter(Statistic.OBJECTS_DELETED);
- OFSPath ofsPath = new OFSPath(path);
- String keyName = ofsPath.getKeyName();
- if (keyName.length() == 0) {
- return false;
- }
+ boolean deleteObjects(OzoneBucket bucket, List<String> keyNameList) {
+ LOG.trace("issuing delete in volume: {}, bucket: {} for keys: {}",
+ bucket.getVolumeName(), bucket.getName(), keyNameList);
+ List<String> keyList = keyNameList.stream()
+ .map(p -> new OFSPath(p).getKeyName())
+ .collect(Collectors.toList());
try {
- bucket.deleteKey(keyName);
+ incrementCounter(Statistic.OBJECTS_DELETED);
Review comment:
I have made the attempt in e65e0d517042ce2056758262b04adc68f989ea8b + 8a01be17ef294eabba10844ad4c18cb05cd2e240. Pls take a look :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1286: HDDS-4040. [OFS] BasicRootedOzoneFileSystem to support batchDelete
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#discussion_r467202871
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
##########
@@ -457,38 +458,79 @@ public boolean deleteObject(String path) {
}
}
+ /**
+ * Helper function to check if the list of key paths are in the same volume
+ * and same bucket.
+ */
+ private boolean areInSameBucket(List<String> keyNameList) {
+ if (keyNameList.size() == 0) {
+ return true;
+ }
+ String firstKeyPath = keyNameList.get(0);
+ final String volAndBucket = new OFSPath(firstKeyPath).getNonKeyPath();
+ // If any key path's volume and bucket from the second element and on
+ // in the list doesn't match the first element's, hasDifferentVolAndBucket
+ // would be true
+ boolean hasDifferentVolAndBucket = keyNameList.stream().skip(1)
+ .anyMatch(p -> !(new OFSPath(p).getNonKeyPath().equals(volAndBucket)));
+ return !hasDifferentVolAndBucket;
+ }
+
/**
* Helper method to delete an object specified by key name in bucket.
*
- * @param pathList key name list to be deleted
+ * Only supports deleting keys in the same bucket in one call.
+ *
+ * Each item in the given list should be the String of an OFS path:
+ * e.g. ofs://om/vol1/buck1/k1
+ *
+ * @param keyNameList key name list to be deleted
* @return true if the key is deleted, false otherwise
*/
@Override
- public boolean deleteObjects(List<String> pathList) {
- // TODO: we will support deleteObjects in ofs.
- LOG.error("ofs currently does not support deleteObjects");
- return false;
+ public boolean deleteObjects(List<String> keyNameList) {
+ LOG.trace("issuing delete for keys: {}", keyNameList);
Review comment:
I removed this trace log from both o3fs and ofs. IMO expanding it needs have to have an extra `if (LOG.isTraceEnabled())` condition to prevent unnecessary evaluation which I think is not worth 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1286: HDDS-4040. [OFS] BasicRootedOzoneFileSystem to support batchDelete
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#discussion_r468173915
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java
##########
@@ -190,14 +190,19 @@ public InputStream readFile(String key) throws IOException {
}
}
+ @Deprecated
protected void incrementCounter(Statistic objectsRead) {
//noop: Use OzoneClientAdapterImpl which supports statistics.
Review comment:
My intention is to encourage new code to explicitly specify `count` since incrementing by 1 is now "old" with the introduction of batch delete / batch rename.
And all the existing calls have been migrated to the new one `incrementCounter(Statistic objectsRead, long count)`.
Actually I can just remove the old method since it is `protected`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on pull request #1286: HDDS-4040. [OFS] BasicRootedOzoneFileSystem to support batchDelete
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#issuecomment-670775889
Thanks @smengcl for the update. Let's merge it by EOD today if there is no additional comments.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1286: HDDS-4040. [OFS] BasicRootedOzoneFileSystem to support batchDelete
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#discussion_r468875440
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneFileSystem.java
##########
@@ -220,17 +220,22 @@ protected InputStream createFSInputStream(InputStream inputStream) {
return new OzoneFSInputStream(inputStream, statistics);
}
+ @Deprecated
protected void incrementCounter(Statistic statistic) {
//don't do anyting in this default implementation.
Review comment:
Updated in 5145690b3ab5404754554229dea0823dea32d202.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1286: HDDS-4040. [OFS] BasicRootedOzoneFileSystem to support batchDelete
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#discussion_r467211897
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
##########
@@ -457,38 +458,79 @@ public boolean deleteObject(String path) {
}
}
+ /**
+ * Helper function to check if the list of key paths are in the same volume
+ * and same bucket.
+ */
+ private boolean areInSameBucket(List<String> keyNameList) {
+ if (keyNameList.size() == 0) {
+ return true;
+ }
+ String firstKeyPath = keyNameList.get(0);
+ final String volAndBucket = new OFSPath(firstKeyPath).getNonKeyPath();
+ // If any key path's volume and bucket from the second element and on
+ // in the list doesn't match the first element's, hasDifferentVolAndBucket
+ // would be true
+ boolean hasDifferentVolAndBucket = keyNameList.stream().skip(1)
+ .anyMatch(p -> !(new OFSPath(p).getNonKeyPath().equals(volAndBucket)));
+ return !hasDifferentVolAndBucket;
+ }
+
/**
* Helper method to delete an object specified by key name in bucket.
*
- * @param pathList key name list to be deleted
+ * Only supports deleting keys in the same bucket in one call.
+ *
+ * Each item in the given list should be the String of an OFS path:
+ * e.g. ofs://om/vol1/buck1/k1
+ *
+ * @param keyNameList key name list to be deleted
* @return true if the key is deleted, false otherwise
*/
@Override
- public boolean deleteObjects(List<String> pathList) {
- // TODO: we will support deleteObjects in ofs.
- LOG.error("ofs currently does not support deleteObjects");
- return false;
+ public boolean deleteObjects(List<String> keyNameList) {
+ LOG.trace("issuing delete for keys: {}", keyNameList);
+ if (keyNameList.size() == 0) {
+ return false;
+ }
+ // Sanity check. Support only deleting a list of keys in the same bucket
+ if (!areInSameBucket(keyNameList)) {
+ LOG.error("Deleting keys from different buckets in a single batch "
+ + "is not supported.");
+ return false;
+ }
+ try {
+ OFSPath firstKeyPath = new OFSPath(keyNameList.get(0));
+ OzoneBucket bucket = getBucket(firstKeyPath, false);
+ return deleteObjects(bucket, keyNameList);
+ } catch (IOException ioe) {
+ LOG.error("delete key failed: {}", ioe.getMessage());
+ return false;
+ }
}
/**
* Package-private helper function to reduce calls to getBucket().
+ *
+ * This will be faster than the public variant of the method since this
+ * doesn't verify the same-bucket condition.
+ *
* @param bucket Bucket to operate in.
- * @param path Path to delete.
- * @return true if operation succeeded, false upon IOException.
+ * @param keyNameList key name list to be deleted.
+ * @return true if operation succeeded, false on IOException.
*/
- boolean deleteObject(OzoneBucket bucket, String path) {
- LOG.trace("issuing delete for path to key: {}", path);
- incrementCounter(Statistic.OBJECTS_DELETED);
- OFSPath ofsPath = new OFSPath(path);
- String keyName = ofsPath.getKeyName();
- if (keyName.length() == 0) {
- return false;
- }
+ boolean deleteObjects(OzoneBucket bucket, List<String> keyNameList) {
+ LOG.trace("issuing delete in volume: {}, bucket: {} for keys: {}",
+ bucket.getVolumeName(), bucket.getName(), keyNameList);
+ List<String> keyList = keyNameList.stream()
+ .map(p -> new OFSPath(p).getKeyName())
+ .collect(Collectors.toList());
try {
- bucket.deleteKey(keyName);
+ incrementCounter(Statistic.OBJECTS_DELETED);
Review comment:
I have made the attempt in e65e0d517042ce2056758262b04adc68f989ea8b. Pls take a look :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on pull request #1286: HDDS-4040. OFS should use deleteObjects when deleting directory
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#issuecomment-670087942
> > Do you know where [this](https://github.com/apache/hadoop-ozone/runs/946974224) 45m-46m timeout is imposed?
>
> Looks like `TestRootedOzoneFileSystem` hits [`surefire.fork.timeout` of 1000 seconds](https://github.com/apache/hadoop-ozone/commit/fc8ae43c173cad67e97995d5c0bc28fa753b3985), as indicated by the error message `There was a timeout or other error in the fork`.
Thanks! I'm increasing it further to 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1286: HDDS-4040. [OFS] BasicRootedOzoneFileSystem to support batchDelete
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#discussion_r467197266
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java
##########
@@ -255,9 +258,11 @@ public FSDataOutputStream append(Path f, int bufferSize,
}
@Override
- boolean processKeyPath(String keyPath) throws IOException {
- String newPath = dstPath.concat(keyPath.substring(srcPath.length()));
- adapterImpl.rename(this.bucket, keyPath, newPath);
+ boolean processKeyPath(List<String> keyPathList) throws IOException {
+ for (String keyPath : keyPathList) {
+ String newPath = dstPath.concat(keyPath.substring(srcPath.length()));
+ adapterImpl.rename(this.bucket, keyPath, newPath);
Review comment:
Filed HDDS-4090
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on pull request #1286: HDDS-4040. OFS should use deleteObjects when deleting directory
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#issuecomment-670088509
>@bharatviswa504 Now that we can prove the tests work with both ozone.om.enable.filesystem.paths options. Should we >proceed with fdbc71c ?
Can we use @BeforeClass for starting MiniOzoneCluster instead for each test, that would save time.
I would like to keep this parameterized, if anything is broken it would be easily caught, as default is false. (because of this, there were some cases missed)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl edited a comment on pull request #1286: HDDS-4040. OFS should use deleteObjects when deleting directory
Posted by GitBox <gi...@apache.org>.
smengcl edited a comment on pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#issuecomment-670082854
I ran `TestRootedOzoneFileSystem` locally again. It seems the problem is it is taking significant longer (double the time spent) to run the test with the parameter (14m 52s total for both, about 7m 30s for each) than without the parameter (7m 47s). Good new is all tests pass.
Another observation. On the GH workflow run, without parameterization we were spending around 9.5min for `TestRootedOzoneFileSystem` without parameterization:
https://github.com/apache/hadoop-ozone/runs/941577903
```
[INFO] Running org.apache.hadoop.fs.ozone.TestOzoneFileSystem
[INFO] Tests run: 10, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 251.496 s - in org.apache.hadoop.fs.ozone.TestOzoneFileSystem
...
[INFO] Running org.apache.hadoop.fs.ozone.TestRootedOzoneFileSystem
[INFO] Tests run: 24, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 566.177 s - in org.apache.hadoop.fs.ozone.TestRootedOzoneFileSystem
...
[INFO] Running org.apache.hadoop.fs.ozone.TestOzoneFileInterfaces
[WARNING] Tests run: 18, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 381.674 s - in org.apache.hadoop.fs.ozone.TestOzoneFileInterfaces
```
With parameterization we can expect it to be spending over 1100s (18min) on this one test suite.. which is not good.
We will need to reduce the run time of the test suite. Probably should do this in another jira, with something like [HDDS-2833](https://github.com/apache/hadoop-ozone/commit/a967de23c0a2bffaef8be1827001315646ef928b#diff-82ba95bf0c9926db3c2da816c87ab896R67) by reducing the overhead of starting up & tearing down the mini cluster.
Also I haven't figured out where the timeout is imposed as of now. @adoroszlai Do you know where [this](https://github.com/apache/hadoop-ozone/runs/946974224) 45m-46m timeout is imposed?
@bharatviswa504 Now that we can prove the tests work with both `ozone.om.enable.filesystem.paths` options. Should we proceed with fdbc71cdabcfa846d538878f80bb602a37a63528 ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl edited a comment on pull request #1286: HDDS-4040. OFS should use deleteObjects when deleting directory
Posted by GitBox <gi...@apache.org>.
smengcl edited a comment on pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#issuecomment-670082854
I ran `TestRootedOzoneFileSystem` locally again. It seems the problem is it is taking significant longer (double the time spent) to run the test with the parameter (14m 52s total for both, about 7m 30s for each) than without the parameter (7m 47s). Good new is all tests pass.
Another observation. On the GH workflow run, without parameterization we were spending around 9.5min for `TestRootedOzoneFileSystem` without parameterization:
https://github.com/apache/hadoop-ozone/runs/941577903
```
[INFO] Running org.apache.hadoop.fs.ozone.TestOzoneFileSystem
[INFO] Tests run: 10, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 251.496 s - in org.apache.hadoop.fs.ozone.TestOzoneFileSystem
...
[INFO] Running org.apache.hadoop.fs.ozone.TestRootedOzoneFileSystem
[INFO] Tests run: 24, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 566.177 s - in org.apache.hadoop.fs.ozone.TestRootedOzoneFileSystem
...
[INFO] Running org.apache.hadoop.fs.ozone.TestOzoneFileInterfaces
[WARNING] Tests run: 18, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 381.674 s - in org.apache.hadoop.fs.ozone.TestOzoneFileInterfaces
```
With parameterization we can expect it to be spending over 1100s (18min) on this one test suite.. which is not good.
We will need to reduce the run time of the test suite. Probably should do this in another jira, with something like [HDDS-2833](https://github.com/apache/hadoop-ozone/commit/a967de23c0a2bffaef8be1827001315646ef928b#diff-82ba95bf0c9926db3c2da816c87ab896R67).
Also I haven't figured out where the timeout is imposed as of now. @adoroszlai Do you know where [this](https://github.com/apache/hadoop-ozone/runs/946974224) 45m-46m timeout is imposed?
@bharatviswa504 Now that we can prove the tests work with both `ozone.om.enable.filesystem.paths` options. Should we proceed with fdbc71cdabcfa846d538878f80bb602a37a63528 ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] adoroszlai commented on a change in pull request #1286: HDDS-4040. [OFS] BasicRootedOzoneFileSystem to support batchDelete
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#discussion_r467368213
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
##########
@@ -301,14 +302,19 @@ public InputStream readFile(String pathStr) throws IOException {
}
}
+ @Deprecated
protected void incrementCounter(Statistic objectsRead) {
//noop: Use OzoneClientAdapterImpl which supports statistics.
Review comment:
```suggestion
protected void incrementCounter(Statistic objectsRead) {
incrementCounter(objectsRead, 1);
```
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java
##########
@@ -190,14 +190,19 @@ public InputStream readFile(String key) throws IOException {
}
}
+ @Deprecated
protected void incrementCounter(Statistic objectsRead) {
//noop: Use OzoneClientAdapterImpl which supports statistics.
Review comment:
Why is this being deprecated? It could delegate to the new method.
```suggestion
protected void incrementCounter(Statistic objectsRead) {
incrementCounter(objectsRead, 1);
```
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
##########
@@ -457,45 +463,83 @@ public boolean deleteObject(String path) {
}
}
+ /**
+ * Helper function to check if the list of key paths are in the same volume
+ * and same bucket.
+ */
+ private boolean areInSameBucket(List<String> keyNameList) {
+ if (keyNameList.size() == 0) {
Review comment:
Nit: Sonar prefers `isEmpty()` over `size() == 0`.
```suggestion
if (keyNameList.isEmpty()) {
```
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneFileSystem.java
##########
@@ -220,17 +220,22 @@ protected InputStream createFSInputStream(InputStream inputStream) {
return new OzoneFSInputStream(inputStream, statistics);
}
+ @Deprecated
protected void incrementCounter(Statistic statistic) {
//don't do anyting in this default implementation.
Review comment:
```suggestion
protected void incrementCounter(Statistic statistic) {
incrementCounter(statistic, 1);
```
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
##########
@@ -457,45 +463,83 @@ public boolean deleteObject(String path) {
}
}
+ /**
+ * Helper function to check if the list of key paths are in the same volume
+ * and same bucket.
+ */
+ private boolean areInSameBucket(List<String> keyNameList) {
+ if (keyNameList.size() == 0) {
+ return true;
+ }
+ String firstKeyPath = keyNameList.get(0);
+ final String volAndBucket = new OFSPath(firstKeyPath).getNonKeyPath();
+ // If any key path's volume and bucket from the second element and on
+ // in the list doesn't match the first element's, hasDifferentVolAndBucket
+ // would be true
+ boolean hasDifferentVolAndBucket = keyNameList.stream().skip(1)
+ .anyMatch(p -> !(new OFSPath(p).getNonKeyPath().equals(volAndBucket)));
+ return !hasDifferentVolAndBucket;
Review comment:
Nit: we could save two negations by using `allMatch()`.
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java
##########
@@ -183,25 +186,30 @@ public String getScheme() {
@Override
public FSDataInputStream open(Path path, int bufferSize) throws IOException {
- incrementCounter(Statistic.INVOCATION_OPEN);
+ incrementCounter(Statistic.INVOCATION_OPEN, 1);
statistics.incrementReadOps(1);
LOG.trace("open() path: {}", path);
final String key = pathToKey(path);
return new FSDataInputStream(
new OzoneFSInputStream(adapter.readFile(key), statistics));
}
+ @Deprecated
protected void incrementCounter(Statistic statistic) {
//don't do anything in this default implementation.
Review comment:
```suggestion
protected void incrementCounter(Statistic statistic) {
incrementCounter(statistic, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1286: HDDS-4040. OFS should use deleteObjects when deleting directory
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#discussion_r466608698
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
##########
@@ -457,38 +458,79 @@ public boolean deleteObject(String path) {
}
}
+ /**
+ * Helper function to check if the list of key paths are in the same volume
+ * and same bucket.
+ */
+ private boolean areInSameBucket(List<String> keyNameList) {
+ if (keyNameList.size() == 0) {
+ return true;
+ }
+ String firstKeyPath = keyNameList.get(0);
+ final String volAndBucket = new OFSPath(firstKeyPath).getNonKeyPath();
+ // If any key path's volume and bucket from the second element and on
+ // in the list doesn't match the first element's, hasDifferentVolAndBucket
+ // would be true
+ boolean hasDifferentVolAndBucket = keyNameList.stream().skip(1)
+ .anyMatch(p -> !(new OFSPath(p).getNonKeyPath().equals(volAndBucket)));
+ return !hasDifferentVolAndBucket;
+ }
+
/**
* Helper method to delete an object specified by key name in bucket.
*
- * @param pathList key name list to be deleted
+ * Only supports deleting keys in the same bucket in one call.
+ *
+ * Each item in the given list should be the String of an OFS path:
+ * e.g. ofs://om/vol1/buck1/k1
+ *
+ * @param keyNameList key name list to be deleted
* @return true if the key is deleted, false otherwise
*/
@Override
- public boolean deleteObjects(List<String> pathList) {
- // TODO: we will support deleteObjects in ofs.
- LOG.error("ofs currently does not support deleteObjects");
- return false;
+ public boolean deleteObjects(List<String> keyNameList) {
+ LOG.trace("issuing delete for keys: {}", keyNameList);
+ if (keyNameList.size() == 0) {
Review comment:
Is there a reason for return false, because BasicOzoneFileSystem does not return false when key list is empty.
[https://github.com/apache/hadoop-ozone/blob/master/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java#L287](codelink)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on pull request #1286: HDDS-4040. OFS should use deleteObjects when deleting directory
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#issuecomment-669513483
It seems adding parameterized test for `TestRootedOzoneFileSystem` tripped the time out.
Before the parameterization it is already taking 40min to finish the integration test suite:
https://github.com/apache/hadoop-ozone/runs/941577903
```
[INFO] Reactor Summary for Apache Hadoop Ozone Integration Tests 0.6.0-SNAPSHOT:
[INFO]
[INFO] Apache Hadoop Ozone Integration Tests .............. SUCCESS [40:01 min]
[INFO] Apache Hadoop Ozone Mini Ozone Chaos Tests ......... SUCCESS [ 1.758 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
```
After the parameterization commit it is timing out at over 45min:
https://github.com/apache/hadoop-ozone/pull/1286/checks?check_run_id=946974224
```
[INFO] Reactor Summary for Apache Hadoop Ozone Integration Tests 0.6.0-SNAPSHOT:
[INFO]
[INFO] Apache Hadoop Ozone Integration Tests .............. FAILURE [45:44 min]
[INFO] Apache Hadoop Ozone Mini Ozone Chaos Tests ......... SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 45:45 min
[INFO] Finished at: 2020-08-04T23:33:52Z
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] adoroszlai commented on pull request #1286: HDDS-4040. [OFS] BasicRootedOzoneFileSystem to support batchDelete
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#issuecomment-672647656
Thanks @smengcl for the contribution, @bharatviswa504 and @xiaoyuyao 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] adoroszlai commented on pull request #1286: HDDS-4040. OFS should use deleteObjects when deleting directory
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#issuecomment-670086505
> Do you know where [this](https://github.com/apache/hadoop-ozone/runs/946974224) 45m-46m timeout is imposed?
Looks like `TestRootedOzoneFileSystem` hits [`surefire.fork.timeout` of 1000 seconds](https://github.com/apache/hadoop-ozone/commit/fc8ae43c173cad67e97995d5c0bc28fa753b3985), as indicated by the error message `There was a timeout or other error in the fork`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1286: HDDS-4040. OFS should use deleteObjects when deleting directory
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#discussion_r467156037
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java
##########
@@ -255,9 +258,11 @@ public FSDataOutputStream append(Path f, int bufferSize,
}
@Override
- boolean processKeyPath(String keyPath) throws IOException {
- String newPath = dstPath.concat(keyPath.substring(srcPath.length()));
- adapterImpl.rename(this.bucket, keyPath, newPath);
+ boolean processKeyPath(List<String> keyPathList) throws IOException {
+ for (String keyPath : keyPathList) {
+ String newPath = dstPath.concat(keyPath.substring(srcPath.length()));
+ adapterImpl.rename(this.bucket, keyPath, newPath);
Review comment:
Do we have a follow up JIRA to fix rename once the batch rename is ready?
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
##########
@@ -457,38 +458,79 @@ public boolean deleteObject(String path) {
}
}
+ /**
+ * Helper function to check if the list of key paths are in the same volume
+ * and same bucket.
+ */
+ private boolean areInSameBucket(List<String> keyNameList) {
+ if (keyNameList.size() == 0) {
+ return true;
+ }
+ String firstKeyPath = keyNameList.get(0);
+ final String volAndBucket = new OFSPath(firstKeyPath).getNonKeyPath();
+ // If any key path's volume and bucket from the second element and on
+ // in the list doesn't match the first element's, hasDifferentVolAndBucket
+ // would be true
+ boolean hasDifferentVolAndBucket = keyNameList.stream().skip(1)
+ .anyMatch(p -> !(new OFSPath(p).getNonKeyPath().equals(volAndBucket)));
+ return !hasDifferentVolAndBucket;
+ }
+
/**
* Helper method to delete an object specified by key name in bucket.
*
- * @param pathList key name list to be deleted
+ * Only supports deleting keys in the same bucket in one call.
+ *
+ * Each item in the given list should be the String of an OFS path:
+ * e.g. ofs://om/vol1/buck1/k1
+ *
+ * @param keyNameList key name list to be deleted
* @return true if the key is deleted, false otherwise
*/
@Override
- public boolean deleteObjects(List<String> pathList) {
- // TODO: we will support deleteObjects in ofs.
- LOG.error("ofs currently does not support deleteObjects");
- return false;
+ public boolean deleteObjects(List<String> keyNameList) {
+ LOG.trace("issuing delete for keys: {}", keyNameList);
Review comment:
LOG.trace a keyNameList address does not provide much useful info. I would suggest either remove or expand the list.
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
##########
@@ -457,38 +458,79 @@ public boolean deleteObject(String path) {
}
}
+ /**
+ * Helper function to check if the list of key paths are in the same volume
+ * and same bucket.
+ */
+ private boolean areInSameBucket(List<String> keyNameList) {
+ if (keyNameList.size() == 0) {
+ return true;
+ }
+ String firstKeyPath = keyNameList.get(0);
+ final String volAndBucket = new OFSPath(firstKeyPath).getNonKeyPath();
+ // If any key path's volume and bucket from the second element and on
+ // in the list doesn't match the first element's, hasDifferentVolAndBucket
+ // would be true
+ boolean hasDifferentVolAndBucket = keyNameList.stream().skip(1)
+ .anyMatch(p -> !(new OFSPath(p).getNonKeyPath().equals(volAndBucket)));
+ return !hasDifferentVolAndBucket;
+ }
+
/**
* Helper method to delete an object specified by key name in bucket.
*
- * @param pathList key name list to be deleted
+ * Only supports deleting keys in the same bucket in one call.
+ *
+ * Each item in the given list should be the String of an OFS path:
+ * e.g. ofs://om/vol1/buck1/k1
+ *
+ * @param keyNameList key name list to be deleted
* @return true if the key is deleted, false otherwise
*/
@Override
- public boolean deleteObjects(List<String> pathList) {
- // TODO: we will support deleteObjects in ofs.
- LOG.error("ofs currently does not support deleteObjects");
- return false;
+ public boolean deleteObjects(List<String> keyNameList) {
+ LOG.trace("issuing delete for keys: {}", keyNameList);
+ if (keyNameList.size() == 0) {
+ return false;
+ }
+ // Sanity check. Support only deleting a list of keys in the same bucket
+ if (!areInSameBucket(keyNameList)) {
+ LOG.error("Deleting keys from different buckets in a single batch "
+ + "is not supported.");
+ return false;
+ }
+ try {
+ OFSPath firstKeyPath = new OFSPath(keyNameList.get(0));
+ OzoneBucket bucket = getBucket(firstKeyPath, false);
+ return deleteObjects(bucket, keyNameList);
+ } catch (IOException ioe) {
+ LOG.error("delete key failed: {}", ioe.getMessage());
+ return false;
+ }
}
/**
* Package-private helper function to reduce calls to getBucket().
+ *
+ * This will be faster than the public variant of the method since this
+ * doesn't verify the same-bucket condition.
+ *
* @param bucket Bucket to operate in.
- * @param path Path to delete.
- * @return true if operation succeeded, false upon IOException.
+ * @param keyNameList key name list to be deleted.
+ * @return true if operation succeeded, false on IOException.
*/
- boolean deleteObject(OzoneBucket bucket, String path) {
- LOG.trace("issuing delete for path to key: {}", path);
- incrementCounter(Statistic.OBJECTS_DELETED);
- OFSPath ofsPath = new OFSPath(path);
- String keyName = ofsPath.getKeyName();
- if (keyName.length() == 0) {
- return false;
- }
+ boolean deleteObjects(OzoneBucket bucket, List<String> keyNameList) {
+ LOG.trace("issuing delete in volume: {}, bucket: {} for keys: {}",
+ bucket.getVolumeName(), bucket.getName(), keyNameList);
+ List<String> keyList = keyNameList.stream()
+ .map(p -> new OFSPath(p).getKeyName())
+ .collect(Collectors.toList());
try {
- bucket.deleteKey(keyName);
+ incrementCounter(Statistic.OBJECTS_DELETED);
Review comment:
should incrementCount for deletes by the number of keys in the list instead of 1?
Also, should we move the counter update if deleteKeys succeed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 edited a comment on pull request #1286: HDDS-4040. OFS should use deleteObjects when deleting directory
Posted by GitBox <gi...@apache.org>.
bharatviswa504 edited a comment on pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#issuecomment-669533265
>It seems adding parameterized test for TestRootedOzoneFileSystem tripped the time out.
Do we need to increase time out in 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1286: HDDS-4040. [OFS] BasicRootedOzoneFileSystem to support batchDelete
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#discussion_r467199383
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
##########
@@ -457,38 +458,79 @@ public boolean deleteObject(String path) {
}
}
+ /**
+ * Helper function to check if the list of key paths are in the same volume
+ * and same bucket.
+ */
+ private boolean areInSameBucket(List<String> keyNameList) {
+ if (keyNameList.size() == 0) {
+ return true;
+ }
+ String firstKeyPath = keyNameList.get(0);
+ final String volAndBucket = new OFSPath(firstKeyPath).getNonKeyPath();
+ // If any key path's volume and bucket from the second element and on
+ // in the list doesn't match the first element's, hasDifferentVolAndBucket
+ // would be true
+ boolean hasDifferentVolAndBucket = keyNameList.stream().skip(1)
+ .anyMatch(p -> !(new OFSPath(p).getNonKeyPath().equals(volAndBucket)));
+ return !hasDifferentVolAndBucket;
+ }
+
/**
* Helper method to delete an object specified by key name in bucket.
*
- * @param pathList key name list to be deleted
+ * Only supports deleting keys in the same bucket in one call.
+ *
+ * Each item in the given list should be the String of an OFS path:
+ * e.g. ofs://om/vol1/buck1/k1
+ *
+ * @param keyNameList key name list to be deleted
* @return true if the key is deleted, false otherwise
*/
@Override
- public boolean deleteObjects(List<String> pathList) {
- // TODO: we will support deleteObjects in ofs.
- LOG.error("ofs currently does not support deleteObjects");
- return false;
+ public boolean deleteObjects(List<String> keyNameList) {
+ LOG.trace("issuing delete for keys: {}", keyNameList);
+ if (keyNameList.size() == 0) {
Review comment:
Good catch. Yes should return true here. Also changed the javadoc a bit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1286: HDDS-4040. [OFS] BasicRootedOzoneFileSystem to support batchDelete
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#discussion_r468877549
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java
##########
@@ -190,14 +190,19 @@ public InputStream readFile(String key) throws IOException {
}
}
+ @Deprecated
protected void incrementCounter(Statistic objectsRead) {
//noop: Use OzoneClientAdapterImpl which supports statistics.
Review comment:
Done. Removed deprecated annotation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on pull request #1286: HDDS-4040. OFS should use deleteObjects when deleting directory
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#issuecomment-669729225
> > It seems adding parameterized test for TestRootedOzoneFileSystem tripped the time out.
>
> Do we need to increase time out in test?
Yep. But the timeout in this case seems to be imposed at a higher level (maybe `forkedProcessTimeoutInSeconds`) not at the test class level (300s for each test method).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1286: HDDS-4040. [OFS] BasicRootedOzoneFileSystem to support batchDelete
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1286:
URL: https://github.com/apache/hadoop-ozone/pull/1286#discussion_r468876907
##########
File path: hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
##########
@@ -301,14 +302,19 @@ public InputStream readFile(String pathStr) throws IOException {
}
}
+ @Deprecated
protected void incrementCounter(Statistic objectsRead) {
//noop: Use OzoneClientAdapterImpl which supports statistics.
Review comment:
I have removed the old method from all Impls. Only kept it in `BasicOzoneFileSystem`/`BasicRootedOzoneFileSystem`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org