Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20725 )
Change subject: KUDU-3527 Fix block manager test when using 64k container block alignment ...................................................................... Patch Set 14: Code-Review+1 (9 comments) Thanks a lot for revving the patch. Almost there, just a few nits and typos to fix. http://gerrit.cloudera.org:8080/#/c/20725/14//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20725/14//COMMIT_MSG@21 PS14, Line 21: RHEL 8.8 ARM 64k core Did you mean "RHEL 8.8 ARM with 64K page size"? http://gerrit.cloudera.org:8080/#/c/20725/14//COMMIT_MSG@36 PS14, Line 36: 64k core nit: 64K page size ? http://gerrit.cloudera.org:8080/#/c/20725/14//COMMIT_MSG@51 PS14, Line 51: Added a jira to find a solution nit: Filed a JIRA issue to track the issue http://gerrit.cloudera.org:8080/#/c/20725/14/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/20725/14/src/kudu/fs/block_manager-test.cc@161 PS14, Line 161: CreateABlock style nit: why not just CreateBlock() ? If you want to emphasize it's a single block, consider naming this method CreateOneBlock() By the code style used in the project, 'a' and 'the' articles aren't used in the method names. You could find the corresponding style guide's section (it calls for consistency in naming) at https://google.github.io/styleguide/cppguide.html#Naming http://gerrit.cloudera.org:8080/#/c/20725/14/src/kudu/fs/block_manager-test.cc@942 PS14, Line 942: is are http://gerrit.cloudera.org:8080/#/c/20725/14/src/kudu/fs/block_manager-test.cc@949 PS14, Line 949: resistent resistant http://gerrit.cloudera.org:8080/#/c/20725/14/src/kudu/fs/block_manager-test.cc@981 PS14, Line 981: // The chance that we delete the : // .metadata but fail to delete the .data file is (1-x) * x I guess that's a bit simplistic given that the deletion of these two files aren't exactly independent (i.e. if there is a failure to delete the first file, we do not even try to delete the second, right?), but it's good enough as an approximation to show what to expect once the test is enabled. Thank you for adding this comment! http://gerrit.cloudera.org:8080/#/c/20725/14/src/kudu/fs/block_manager-test.cc@1022 PS14, Line 1022: , nit: remove the comma? http://gerrit.cloudera.org:8080/#/c/20725/14/src/kudu/fs/block_manager-test.cc@1028 PS14, Line 1028: " nit: remove the stray quote? -- To view, visit http://gerrit.cloudera.org:8080/20725 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7e325bde502b7d7f39dd17fa84cb7eb42a3d7c20 Gerrit-Change-Number: 20725 Gerrit-PatchSet: 14 Gerrit-Owner: Zoltan Martonka <zmarto...@cloudera.com> Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Ashwani Raina <ara...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka <zmarto...@cloudera.com> Gerrit-Comment-Date: Fri, 26 Jan 2024 23:21:41 +0000 Gerrit-HasComments: Yes
