Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20725 )
Change subject: KUDU-3527 Fix block manager test on 64k filesystems ...................................................................... Patch Set 7: (9 comments) Overall looks good to me, but I think this patch needs a bit of clean-up at least easier comprehension later on when it's time to enable the test. http://gerrit.cloudera.org:8080/#/c/20725/7/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/20725/7/src/kudu/fs/block_manager-test.cc@928 PS7, Line 928: const int kNumTries = 3; : const int kNumBlockTries = 500; : const int kNumAppends = 4; nit: could these be constexpr? http://gerrit.cloudera.org:8080/#/c/20725/7/src/kudu/fs/block_manager-test.cc@937 PS7, Line 937: With non-zero crash_on_eio nit: With crash_on_eio enabled http://gerrit.cloudera.org:8080/#/c/20725/7/src/kudu/fs/block_manager-test.cc@939 PS7, Line 939: It is not the : // scope of this test to test this case. This is looks quite confusing given what this test scenario is doing. Instead of this, maybe it's better to add a comment at line 926 to explain why this test is here and why it's added as a disabled one? Overall, please update this block comment, clarifying on the following points: * the 'what' part: what this test scenario is doing * the 'why' part: why this test scenario is here and why it is added as a disabled one? * the 'when' part: when this test scenario should be enabled, if ever? You might consider splitting it in multiple piece, placing some of them at line 926, prefacing the scenario. http://gerrit.cloudera.org:8080/#/c/20725/7/src/kudu/fs/block_manager-test.cc@942 PS7, Line 942: arm nit ARM http://gerrit.cloudera.org:8080/#/c/20725/7/src/kudu/fs/block_manager-test.cc@942 PS7, Line 942: like the 64k option of rhel 8.8 What are those exactly? http://gerrit.cloudera.org:8080/#/c/20725/7/src/kudu/fs/block_manager-test.cc@951 PS7, Line 951: If you set a lower value, the test becomes flaky. With 0.2 it almost always fails. This might become confusing later on when the root cause of KUDU-3528 is addressed: it's not very likely that this comment will be fixed once the DISABLED_ prefix is removed. Maybe, just mention what exactly happens here, not resorting to 'flaky' and 'test failure' terms? I'd think of explaining that the scenario ends up with non-empty .data file and no .meta file much less often if FLAGS_env_inject_eio set to a value less than 0.2. http://gerrit.cloudera.org:8080/#/c/20725/7/src/kudu/fs/block_manager-test.cc@958 PS7, Line 958: the result nit: the result block ID http://gerrit.cloudera.org:8080/#/c/20725/7/src/kudu/fs/block_manager-test.cc@982 PS7, Line 982: if (s.ok()) { : InsertOrDie(&ids, id); : num_created++; : } If this is just a way to overcome injected IO errors, why not to set FLAGS_env_inject_eio = 0 in the scope while calling create_a_block()? Otherwise, please add a comment explaining why it's important to have IO error injections enabled when creating blocks. http://gerrit.cloudera.org:8080/#/c/20725/7/src/kudu/fs/block_manager-test.cc@999 PS7, Line 999: if (it != ids.end()) { : it++; : } nit for here and elsewhere: it would be great if you preserved more of the original comments from the TestMetadataOkayDespiteFailure scenario that are still helpful to the reader to understand what's the intention behind some of these actions. -- 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: 7 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, 19 Jan 2024 23:55:43 +0000 Gerrit-HasComments: Yes
