HDDS-3286. BasicOzoneFileSystem support batchDelete. #814

captainzmc · 2020-04-13T10:07:37Z

What changes were proposed in this pull request?

Currently delete file is to get all the keys in the directory, and then delete one by one. This makes for poor performance.
By tested the deletion directory with 100,000 files, which took 3718.70 sec. And rename it took 7327.936 sec.
Using this PR, when batch-size is set to 100, the time of delete and rename's directory of 100,000 files is 62.498 sec and 46.002 sec. Performance improved nearly 100 times。

Rename has the same problem, and I'll add rename to the other PR.

##What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3286

How was this patch tested?

Added UT
Existing acceptance tests.

mukul1987 · 2020-04-13T12:27:34Z

Please have a look at https://issues.apache.org/jira/browse/HDDS-2939. The problem is being addressed there.

rakeshadr

Thanks for the good work.
Added few suggestions and comments. Please go through it. Thanks!

hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java

hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java

rakeshadr · 2020-04-13T16:24:50Z

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java

Do we need String keyName argument ? Can you please incorporate String keyName argument into the keyNameList argument, something similar you have very well refactored for keyNameList.add(keyName); in deleteKey api.

Do we need String keyName argument ? Can you please incorporate String keyName argument into the keyNameList argument, something similar you have very well refactored for keyNameList.add(keyName); in deleteKey api.

The keyName in OmKeyArgs is Shared with other interfaces. Operations like create and list still need to use it.

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequest.java

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/RenameInfo.java

hadoop-ozone/ozonefs/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneFileSystem.java

...one-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyRenameRequest.java

captainzmc · 2020-04-15T09:22:43Z

Please have a look at https://issues.apache.org/jira/browse/HDDS-2939. The problem is being addressed there.

@mukul1987 Thanks for the tip. After looking at the design documentation, I found that is a better implementation. I'm going to colse this PR.

captainzmc · 2020-04-15T09:26:00Z

Thanks for the good work.
Added few suggestions and comments. Please go through it. Thanks!

Hi @rakeshadr Thank for you review. By looking at the design documentation of https://issues.apache.org/jira/browse/HDDS-2939. I found that the HDDS-2939 works better, and I'm going to colse this PR.

captainzmc · 2020-05-19T12:26:56Z

hi @rakeshadr Thank for you review.
After negotiation, I re-opened this PR, and I modified it according to your suggestion. In addition, for batch optimization of rename, I plan to submit a PR hdds-3620 separately based on this PR.

captainzmc · 2020-05-19T12:39:00Z

Hi @ChenSammi @xiaoyuyao
As we discussed before, because the progress of HDDS-2939 was slow, and it's going to take a long time to merge into master. So we optimized the batch operation based on the current interface. We were able to get better performance with this PR before the HDDS-2939 merged.
In addition, for batch rename operation, I plan to submit a PR HDDS-3620 separately based on this.
Could you please help me review this?

timmylicheng · 2020-05-22T07:26:00Z

hadoop-ozone/common/src/main/proto/OzoneManagerProtocol.proto

Can we have a list of KeyArgs here?

Agree. Use list of KeyArgs instead of add a new keyNameList field.

Very good advice. Instead of modifying KeyArgs, I'll try to implement delete with a List that contains KeyArgs.

timmylicheng · 2020-05-22T07:26:28Z

hadoop-ozone/common/src/main/proto/OzoneManagerProtocol.proto

Why not remain KeyArgs as for single key?

Yes, We can avoid changing it here，by use list of KeyArgs.

timmylicheng · 2020-05-22T07:26:47Z

hadoop-ozone/common/src/main/proto/OzoneManagerProtocol.proto

Can we use repeated KeyArgs here?

timmylicheng · 2020-05-22T07:30:33Z

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java

We need to iterate all omKeyInfo in the list as well here

timmylicheng · 2020-05-22T07:33:42Z

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java

Not related to this class. But you wanna visit TestOzoneManagerHAWithData and see if HA needs a test case for deleting a list of keys.

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java

timmylicheng · 2020-05-22T07:49:15Z

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java

It would be great if we add a failure case here like there is an unknown key in a list of known keys. We would like to also test exceptions.

captainzmc · 2020-05-22T08:17:52Z

Hi @bharatviswa504, Could you please help to review this PR? Let's see if there is any effect on OM HA.

hadoop-ozone/common/src/main/proto/OzoneManagerProtocol.proto

xiaoyuyao · 2020-05-22T16:27:16Z

hadoop-ozone/common/src/main/proto/OzoneManagerProtocol.proto

why do we need to change toKeyName from required to optional?

This place is useless for modification, I will restore this place.

xiaoyuyao · 2020-05-22T16:30:10Z

hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java

When we delete a list of keys, upon failure in the middle, can we return a list of deleted keys and undeleted keys? This may not be an issue when you delete a single key but when batch deleting, it is hard to recover from the failures without that information.

Agree with Xiaoyu, we should return a list of deleted key result on failure.

I will add unDeletedKeys and deletedKeys in exception when an exception occurs when delete keys.

xiaoyuyao · 2020-05-22T16:34:36Z

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java

This is OK with o3fs as we will mount a single bucket. In the context of ofs where you can have multiple volume buckets under the root. This lock can't guarantee atomic across all the delete keyname list.

Here I will make sure that all buckets I used are locked to ensure compatibility with ofs.

xiaoyuyao · 2020-05-22T16:37:41Z

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java

This will only print the address of the keNameList. You may want to expand the list and also protect the LOG.debug with a if LOG.isDebugEnabled().

xiaoyuyao · 2020-05-22T16:38:34Z

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java

same as above. we might only want to print the key that failed in the deletion instead of the whole list upon failures. print the whole list can be a debug or trace level log.

xiaoyuyao · 2020-05-22T16:42:55Z

hadoop-ozone/ozonefs/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java

Should we define a new method called deleteObjects to be backward compatible?

captainzmc · 2020-06-03T02:44:52Z

Fixed issues with large lock granularity. PR can continue to be reviewed.

captainzmc · 2020-06-05T13:40:13Z

Had rebased master and fixed conflict. PR can continue to be reviewed. cc @timmylicheng @ChenSammi @xiaoyuyao Thanks.

ChenSammi

Sugguest to change all deleteBatchKey liked namning to deleleKeys.

ChenSammi · 2020-06-09T03:32:18Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java


+  public static final String OZONE_FS_ITERATE_BATCH_SIZE =
+          "ozone.fs.iterate.batch-size";
+  public static final int OZONE_FS_ITERATE_BATCH_SIZE_DEFAULT = 1;


Suggest a bigger default size, say 100?

ChenSammi · 2020-06-09T03:32:40Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java

      "ozone.s3.token.max.lifetime";
  public static final String OZONE_S3_AUTHINFO_MAX_LIFETIME_KEY_DEFAULT = "3m";

+  public static final String OZONE_FS_ITERATE_BATCH_SIZE =


Will this property be used for batch rename too?

Yes, batch Rename will also use this property.

ChenSammi · 2020-06-09T03:34:10Z

hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java

+   * @throws IOException
+   */
+  void deleteKeys(String volumeName, String bucketName,
+                     List<String> keyNameList)


Indent is incorrect.

ChenSammi · 2020-06-09T03:38:09Z

hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java

Agree with Xiaoyu, we should return a list of deleted key result on failure.

...ne-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMBatchKeyDeleteRequest.java

codecov-commenter · 2020-06-12T08:28:15Z

Codecov Report

❗ No coverage uploaded for pull request base (master@26d8375). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #814   +/-   ##
=========================================
  Coverage          ?   69.43%           
  Complexity        ?     9125           
=========================================
  Files             ?      962           
  Lines             ?    48287           
  Branches          ?     4694           
=========================================
  Hits              ?    33528           
  Misses            ?    12541           
  Partials          ?     2218

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26d8375...8604295. Read the comment docs.

captainzmc · 2020-06-18T09:44:14Z

Status updates:
Some changes have been made based on the new comments. This PR can continue to be reviewed.

hadoop-ozone/common/src/main/proto/OzoneManagerProtocol.proto

...op-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/OMClientRequest.java

captainzmc · 2020-06-23T10:14:59Z

Thanks @xiaoyuyao for starting to review this PR again. I have made some modifications according to the suggestions above. In addition, Due to the conflict problem, I rebase the previous commit.
So, this PR can continue to be reviewed.

xiaoyuyao · 2020-06-23T21:12:32Z

Thanks @captainzmc for the update. The latest change LGTM, +1.

* upstream/master: (56 commits) HDDS-3264. Fix TestCSMMetrics.java. (apache#1120) HDDS-3858. Remove support to start Ozone and HDFS datanodes in the same JVM (apache#1117) HDDS-3704. Update all the documentation to use ozonefs-hadoop2/3 instead of legacy/current (apache#1099) HDDS-3773. Add OMDBDefinition to define structure of om.db. (apache#1076) Revert "HDDS-3263. Fix TestCloseContainerByPipeline.java. (apache#1119)" (apache#1126) HDDS-3821. Disable Ozone SPNEGO should not fall back to hadoop.http.a… (apache#1101) HDDS-3819. OzoneManager#listVolumeByUser ignores userName parameter when ACL is enabled (apache#1087) HDDS-3779. Add csi interface documents to show how to use ozone csi (apache#1059) HDDS-3857. Datanode in compose/ozonescripts can't be started (apache#1116) HDDS-3430. Enable TestWatchForCommit test cases. (apache#1114) HDDS-3263. Fix TestCloseContainerByPipeline.java. (apache#1119) HDDS-3512. s3g multi-part-upload saved incorrect content using streaming (apache#1092) HDDS-3836. Modify ContainerPlacementPolicyFactory JavaDoc (apache#1097) HDDS-3780. Replace the imagePullPolicy from always to IfNotPresent (apache#1055) HDDS-3847. Change OMNotLeaderException logging to DEBUG (apache#1118) HDDS-3745. Improve OM and SCM performance with 64% by avoid collect datanode information to s3g (apache#1031) HDDS-3286. BasicOzoneFileSystem support batchDelete. (apache#814) HDDS-3850. Update the admin document to let user know how to show the status of all rules. (apache#1109) HDDS-3848. Add ratis.thirdparty.version in main pom.xml (apache#1108) HDDS-3815. Avoid buffer copy in ContainerCommandRequestProto. (apache#1085) ...

rakeshadr reviewed Apr 13, 2020

View reviewed changes

captainzmc closed this Apr 15, 2020

captainzmc reopened this May 10, 2020

captainzmc closed this May 10, 2020

captainzmc changed the title ~~HDDS-3286. BasicOzoneFileSystem support batchDelete and batchRename.~~ (WIP)HDDS-3286. BasicOzoneFileSystem support batchDelete and batchRename. May 18, 2020

captainzmc reopened this May 18, 2020

captainzmc force-pushed the HDDS-3286-new branch 2 times, most recently from c927d2b to 6208606 Compare May 19, 2020 06:39

captainzmc changed the title ~~(WIP)HDDS-3286. BasicOzoneFileSystem support batchDelete and batchRename.~~ (WIP)HDDS-3286. BasicOzoneFileSystem support batchDelete. May 19, 2020

captainzmc changed the title ~~(WIP)HDDS-3286. BasicOzoneFileSystem support batchDelete.~~ HDDS-3286. BasicOzoneFileSystem support batchDelete. May 19, 2020

timmylicheng reviewed May 22, 2020

View reviewed changes

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java Outdated Show resolved Hide resolved

timmylicheng reviewed May 22, 2020

View reviewed changes

xiaoyuyao reviewed May 22, 2020

View reviewed changes

hadoop-ozone/common/src/main/proto/OzoneManagerProtocol.proto Outdated Show resolved Hide resolved

xiaoyuyao reviewed May 22, 2020

View reviewed changes

captainzmc reopened this Jun 2, 2020

nandakumar131 assigned captainzmc Jun 4, 2020

nandakumar131 added the client label Jun 4, 2020

captainzmc force-pushed the HDDS-3286-new branch from a543260 to fd75e92 Compare June 5, 2020 09:24

ChenSammi reviewed Jun 9, 2020

View reviewed changes

captainzmc closed this Jun 9, 2020

captainzmc reopened this Jun 9, 2020

captainzmc force-pushed the HDDS-3286-new branch from 0a2851d to 8604295 Compare June 12, 2020 02:02

captainzmc closed this Jun 12, 2020

captainzmc reopened this Jun 12, 2020

captainzmc force-pushed the HDDS-3286-new branch from 691eca8 to 3f720f1 Compare June 18, 2020 06:42

captainzmc closed this Jun 18, 2020

captainzmc reopened this Jun 18, 2020

captainzmc force-pushed the HDDS-3286-new branch from 3f720f1 to 257a8d4 Compare June 18, 2020 08:11

xiaoyuyao reviewed Jun 19, 2020

View reviewed changes

hadoop-ozone/common/src/main/proto/OzoneManagerProtocol.proto Outdated Show resolved Hide resolved

xiaoyuyao reviewed Jun 19, 2020

View reviewed changes

...op-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/OMClientRequest.java Outdated Show resolved Hide resolved

captainzmc force-pushed the HDDS-3286-new branch from 9bf3f10 to 078c9d4 Compare June 23, 2020 07:48

captainzmc added 2 commits June 23, 2020 16:29

support batch delete and fix review issues.

b377791

fix review issues and resolve the conflict.

44c5788

captainzmc force-pushed the HDDS-3286-new branch from 078c9d4 to 44c5788 Compare June 23, 2020 09:04

xiaoyuyao merged commit a5556d3 into apache:master Jun 23, 2020

captainzmc deleted the HDDS-3286-new branch July 17, 2020 12:55

captainzmc mentioned this pull request Jul 21, 2020

HDDS-3903. OzoneRpcClient support batch rename keys. #1150

Merged

HDDS-3286. BasicOzoneFileSystem support batchDelete. #814

HDDS-3286. BasicOzoneFileSystem support batchDelete. #814

Uh oh!

Conversation

captainzmc commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

mukul1987 commented Apr 13, 2020

Uh oh!

rakeshadr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

captainzmc commented Apr 15, 2020

Uh oh!

captainzmc commented Apr 15, 2020

Uh oh!

captainzmc commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

captainzmc commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

captainzmc commented May 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

captainzmc commented Apr 13, 2020 •

edited

Loading

captainzmc commented May 19, 2020 •

edited

Loading

captainzmc commented May 19, 2020 •

edited

Loading

captainzmc commented May 22, 2020 •

edited

Loading