Ashwani Raina 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 9: (10 comments) Overall looks good to me. Just a few nits. http://gerrit.cloudera.org:8080/#/c/20725/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20725/9//COMMIT_MSG@53 PS9, Line 53: ARM Not related to this change. Just for my understanding. On ARM, what is the out-of-the-box underlying filesystem type? Do we end up using LBM or File block manager on ARM? http://gerrit.cloudera.org:8080/#/c/20725/9/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/20725/9/src/kudu/fs/block_manager-test.cc@933 PS9, Line 933: Bolck nit: Block http://gerrit.cloudera.org:8080/#/c/20725/9/src/kudu/fs/block_manager-test.cc@935 PS9, Line 935: because it can't handle a .data file Maybe we can write: because it can't handle a .data file whose corresponding .metadata file is not present. http://gerrit.cloudera.org:8080/#/c/20725/9/src/kudu/fs/block_manager-test.cc@939 PS9, Line 939: smarted delete Not sure what "smarted delete" means. If you are pointing to approach of "first moving file to an intermediate/hidden state and then delete", then you may want to mention that in detailed words here, for better understanding. Or you can simply mention that comment needs to be rephrased/revisited after KUDU-3528 is fixed. http://gerrit.cloudera.org:8080/#/c/20725/9/src/kudu/fs/block_manager-test.cc@959 PS9, Line 959: auto create_a_block = [&](BlockId* out, const string& test_data) { : unique_ptr<WritableBlock> block; : RETURN_NOT_OK(this->bm_->CreateBlock(this->test_block_opts_, &block)); : RETURN_NOT_OK(block->Append(test_data)); : RETURN_NOT_OK(block->Finalize()); : RETURN_NOT_OK(block->Close()); : *out = block->id(); : return Status::OK(); : }; Do you think it makes sense to move this into a class method that can be called from different tests? http://gerrit.cloudera.org:8080/#/c/20725/9/src/kudu/fs/block_manager-test.cc@982 PS9, Line 982: container nit: containers http://gerrit.cloudera.org:8080/#/c/20725/9/src/kudu/fs/block_manager-test.cc@982 PS9, Line 982: of remove http://gerrit.cloudera.org:8080/#/c/20725/9/src/kudu/fs/block_manager-test.cc@983 PS9, Line 983: with is http://gerrit.cloudera.org:8080/#/c/20725/9/src/kudu/fs/block_manager-test.cc@984 PS9, Line 984: chance remove http://gerrit.cloudera.org:8080/#/c/20725/9/src/kudu/fs/block_manager-test.cc@984 PS9, Line 984: 0.99999999999999999999999999999994174 Add a one-liner to show how you arrived with this calculation. Haven't verified it, but maybe something like this: [1 - (0.75)^250] ~ 0.99999999999999999999999999999994174 -- 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: 9 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: Thu, 25 Jan 2024 13:26:54 +0000 Gerrit-HasComments: Yes
