Zoltan Martonka 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 7: (8 comments) http://gerrit.cloudera.org:8080/#/c/20725/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20725/7//COMMIT_MSG@10 PS7, Line 10: fs_block_size > What's "fs_block_size"? Is it indeed the filesystem's block size (i.e. wha I will use "container block alignment". (it does not matter if it's st_blksize or f_bsize or some other value). 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? thanks. 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. Inst I changed/simplified the test. 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? I will change it to "If we use 64k block alignment (e.g RHEL ARM 8.8 64k core)" 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 ad I try to give a better explanation. Also the root cause of the error is not that a container becomes full, but that we also delete all blocks. 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_ It was important in the original test. It is not important here (but also not a problem). I change the test to only have errors at deletion. Thank you for the suggestion. 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 Nothing. The original test tested what happens of we delete every 2nd block. So I copied it. It is better to delete all blocks. The problematic behaviour will occur more often. I change it. http://gerrit.cloudera.org:8080/#/c/20725/7/src/kudu/fs/block_manager-test.cc@1033 PS7, Line 1033: FLAGS_log_container_max_size = 1024 * 1024; This is not a good fix.I think there still is a chance around 1e-5 that we will be left with a .data file without .metadata with this setting. I obviously can't test it in a reasonable time, but I will revert to the flag solution. Please read the new commit message for more info. -- 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: Wed, 24 Jan 2024 18:47:29 +0000 Gerrit-HasComments: Yes
