Skip to content

Conversation

@lokeshj1703
Copy link
Contributor

Currently in listStatus we make multiple getFileStatus calls. This can be optimized by converting to a single rpc call for listStatus.

Also currently listStatus has to traverse a directory recursively in order to list its immediate children. This happens because in OzoneManager all the metadata is stored in rocksdb sorted on keynames. The Jira also aims to fix this by using seek api provided by rocksdb.

@lokeshj1703 lokeshj1703 requested a review from mukul1987 April 29, 2019 10:26
@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 58 Docker mode activated.
_ Prechecks _
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 3 new or modified test files.
_ trunk Compile Tests _
0 mvndep 35 Maven dependency ordering for branch
+1 mvninstall 1358 trunk passed
+1 compile 146 trunk passed
+1 checkstyle 56 trunk passed
+1 mvnsite 157 trunk passed
+1 shadedclient 1039 branch has no errors when building and testing our client artifacts.
+1 findbugs 214 trunk passed
+1 javadoc 126 trunk passed
_ Patch Compile Tests _
0 mvndep 15 Maven dependency ordering for patch
+1 mvninstall 138 the patch passed
+1 compile 120 the patch passed
+1 cc 120 the patch passed
+1 javac 120 the patch passed
-0 checkstyle 28 hadoop-ozone: The patch generated 6 new + 0 unchanged - 0 fixed = 6 total (was 0)
+1 mvnsite 127 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 920 patch has no errors when building and testing our client artifacts.
+1 findbugs 247 the patch passed
+1 javadoc 113 the patch passed
_ Other Tests _
+1 unit 43 common in the patch passed.
+1 unit 33 client in the patch passed.
-1 unit 75 ozone-manager in the patch failed.
-1 unit 171 ozonefs in the patch failed.
+1 asflicense 31 The patch does not generate ASF License warnings.
5245
Reason Tests
Failed junit tests hadoop.ozone.om.ratis.TestOzoneManagerRatisServer
Subsystem Report/Notes
Docker Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-782/1/artifact/out/Dockerfile
GITHUB PR #782
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle cc
uname Linux 24af074b3c69 4.4.0-141-generic #167~14.04.1-Ubuntu SMP Mon Dec 10 13:20:24 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / b434f55
maven version: Apache Maven 3.3.9
Default Java 1.8.0_191
findbugs v3.1.0-RC1
checkstyle https://builds.apache.org/job/hadoop-multibranch/job/PR-782/1/artifact/out/diff-checkstyle-hadoop-ozone.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-782/1/artifact/out/patch-unit-hadoop-ozone_ozone-manager.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-782/1/artifact/out/patch-unit-hadoop-ozone_ozonefs.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-782/1/testReport/
Max. process+thread count 2720 (vs. ulimit of 5500)
modules C: hadoop-ozone/common hadoop-ozone/client hadoop-ozone/ozone-manager hadoop-ozone/ozonefs U: hadoop-ozone
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-782/1/console
Powered by Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great optimization.
Should this point to first character in the ASCII table ? Also lets verify that this for UTF-8 encoding as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first character in the ASCII table would not work in our case.
The second commit uses codec of RDBStore for handling the UTF-8 case.

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 44 Docker mode activated.
_ Prechecks _
+1 dupname 1 No case conflicting files found.
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 3 new or modified test files.
_ trunk Compile Tests _
0 mvndep 62 Maven dependency ordering for branch
+1 mvninstall 408 trunk passed
+1 compile 197 trunk passed
+1 checkstyle 51 trunk passed
+1 mvnsite 0 trunk passed
+1 shadedclient 883 branch has no errors when building and testing our client artifacts.
+1 javadoc 125 trunk passed
0 spotbugs 238 Used deprecated FindBugs config; considering switching to SpotBugs.
+1 findbugs 420 trunk passed
_ Patch Compile Tests _
0 mvndep 23 Maven dependency ordering for patch
+1 mvninstall 400 the patch passed
+1 compile 202 the patch passed
+1 cc 202 the patch passed
+1 javac 202 the patch passed
-0 checkstyle 28 hadoop-ozone: The patch generated 6 new + 0 unchanged - 0 fixed = 6 total (was 0)
+1 mvnsite 0 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 712 patch has no errors when building and testing our client artifacts.
+1 javadoc 121 the patch passed
+1 findbugs 441 the patch passed
_ Other Tests _
-1 unit 146 hadoop-hdds in the patch failed.
-1 unit 1145 hadoop-ozone in the patch failed.
+1 asflicense 34 The patch does not generate ASF License warnings.
6586
Subsystem Report/Notes
Docker Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-782/2/artifact/out/Dockerfile
GITHUB PR #782
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle cc
uname Linux 157d5e6e41e7 4.4.0-141-generic #167~14.04.1-Ubuntu SMP Mon Dec 10 13:20:24 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / 36267b6
Default Java 1.8.0_191
checkstyle https://builds.apache.org/job/hadoop-multibranch/job/PR-782/2/artifact/out/diff-checkstyle-hadoop-ozone.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-782/2/artifact/out/patch-unit-hadoop-hdds.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-782/2/artifact/out/patch-unit-hadoop-ozone.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-782/2/testReport/
Max. process+thread count 4275 (vs. ulimit of 5500)
modules C: hadoop-hdds/common hadoop-ozone/client hadoop-ozone/common hadoop-ozone/ozone-manager hadoop-ozone/ozonefs U: .
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-782/2/console
versions git=2.7.4 maven=3.3.9 findbugs=3.1.0-RC1
Powered by Apache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we move the acquireBucketLock inside the try block? The original pattern seems good to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be moved out of try block.

Copy link
Contributor

@xiaoyuyao xiaoyuyao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall. A few comments added inline

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keyName can be used here, as keyPath was derived from that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments here do not match with the assert in the next line. Can you please have a look.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment here to explain this line

@lokeshj1703
Copy link
Contributor Author

@xiaoyuyao @mukul1987 I have made addresses the review comments in the latest commit.

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 525 Docker mode activated.
_ Prechecks _
+1 dupname 1 No case conflicting files found.
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 3 new or modified test files.
_ trunk Compile Tests _
0 mvndep 66 Maven dependency ordering for branch
+1 mvninstall 405 trunk passed
+1 compile 207 trunk passed
+1 checkstyle 53 trunk passed
+1 mvnsite 0 trunk passed
+1 shadedclient 829 branch has no errors when building and testing our client artifacts.
+1 javadoc 128 trunk passed
0 spotbugs 236 Used deprecated FindBugs config; considering switching to SpotBugs.
+1 findbugs 417 trunk passed
_ Patch Compile Tests _
0 mvndep 28 Maven dependency ordering for patch
+1 mvninstall 398 the patch passed
+1 compile 212 the patch passed
+1 cc 212 the patch passed
+1 javac 212 the patch passed
-0 checkstyle 29 hadoop-ozone: The patch generated 5 new + 0 unchanged - 0 fixed = 5 total (was 0)
+1 mvnsite 0 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 663 patch has no errors when building and testing our client artifacts.
+1 javadoc 123 the patch passed
+1 findbugs 433 the patch passed
_ Other Tests _
-1 unit 150 hadoop-hdds in the patch failed.
-1 unit 1309 hadoop-ozone in the patch failed.
+1 asflicense 40 The patch does not generate ASF License warnings.
7626
Subsystem Report/Notes
Docker Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-782/3/artifact/out/Dockerfile
GITHUB PR #782
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle cc
uname Linux 4cd11d86f21c 4.4.0-138-generic #164-Ubuntu SMP Tue Oct 2 17:16:02 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / de01422
Default Java 1.8.0_212
checkstyle https://builds.apache.org/job/hadoop-multibranch/job/PR-782/3/artifact/out/diff-checkstyle-hadoop-ozone.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-782/3/artifact/out/patch-unit-hadoop-hdds.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-782/3/artifact/out/patch-unit-hadoop-ozone.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-782/3/testReport/
Max. process+thread count 4646 (vs. ulimit of 5500)
modules C: hadoop-hdds/common hadoop-ozone/client hadoop-ozone/common hadoop-ozone/ozone-manager hadoop-ozone/ozonefs U: .
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-782/3/console
versions git=2.7.4 maven=3.3.9 findbugs=3.1.0-RC1
Powered by Apache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

@mukul1987
Copy link
Contributor

+1, the patch looks good to me.

@lokeshj1703 lokeshj1703 merged commit c1d7d68 into apache:trunk May 21, 2019
@lokeshj1703 lokeshj1703 deleted the HDDS-1461 branch May 21, 2019 09:17
@lokeshj1703
Copy link
Contributor Author

@mukul1987 @xiaoyuyao Thanks for reviewing the PR! I have merged it to trunk.

shanthoosh pushed a commit to shanthoosh/hadoop that referenced this pull request Oct 15, 2019
… method.

Author: Boris S <[email protected]>
Author: Boris S <[email protected]>
Author: Boris Shkolnik <[email protected]>

Reviewers: Bharath Kumarasubramanian <[email protected]>

Closes apache#782 from sborya/removeExtendedSystemAdmin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants