Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20787 )
Change subject: [compaction] Add memory estimation unit test ...................................................................... Patch Set 6: (10 comments) http://gerrit.cloudera.org:8080/#/c/20787/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20787/6//COMMIT_MSG@9 PS6, Line 9: FLAGS_rowset_compaction_memory_estimate_enabled was always set to : false during testing, so this part of the code was not executed in : testing environment Indeed! I recall that when I was working on 1556a353e, I verified the code worked with the data captured at a real system, but then I cleaned up the units for the newly introduced flags, and screwed up, forgetting to update the factor as needed. Ashwani fixed the issue with ae7b08c00, but it's great to eventually have a test for this. Thank you very much for adding the test! http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/compaction_policy-test.cc File src/kudu/tablet/compaction_policy-test.cc: http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/compaction_policy-test.cc@648 PS6, Line 648: TestMemoryEstimation nit: 'Test' is already a part of the name of the test itself ('TestCompactionPolicy'); consider not duplicating it in the name of the scenario, i.e. renaming this into 'MemoryEstimation' http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/compaction_policy-test.cc@651 PS6, Line 651: 2 Why is this magic '2' factor? Is it possible to get rid of it, correspondingly updating the sizes of mock rowsets below? http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/compaction_policy-test.cc@651 PS6, Line 651: *1024*1024 style nit for here and below: separate operation symbol from the operands by space http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/compaction_policy-test.cc@658 PS6, Line 658: 20 Is it crucial to have this factor 20? Isn't the compaction size estimation good enough to work as expected with a couple of rowsets each of memory_limit_hard_bytes size? http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/compaction_policy-test.cc@663 PS6, Line 663: picked.size(), 2 nit for here and below: the expected size comes first, otherwise it's harder to decipher the error message when the assertion triggers http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/compaction_policy-test.cc@672 PS6, Line 672: for (const auto& str : post_capture_logs.logged_msgs()) { : sum += str; : } : ASSERT_STR_CONTAINS(sum, "removed from compaction input due to memory constraints"); Consider using JoinStrings() from strings/join.h Isn't it enough just to search for the pattern in each logged message? I'd think there isn't any concurrency involved here, so it would work, no? http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/diskrowset.cc File src/kudu/tablet/diskrowset.cc: http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/diskrowset.cc@794 PS6, Line 794: uint64_t DiskRowSet::OnDiskDeltaSize() const { Once this new method is introduced, why not to update the code in Tablet::DoMergeCompactionOrFlush(), removing the down-casting? http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/rowset.cc File src/kudu/tablet/rowset.cc: http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/rowset.cc@266 PS6, Line 266: const shared_ptr<RowSet> &rs style nit: the ampersand sticks to the type, not the variable Also, consider using 'const auto&' instead. http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/rowset.cc@266 PS6, Line 266: new_rowsets_ What about 'old_rowsets_'? Those also consume some disk space, no? -- To view, visit http://gerrit.cloudera.org:8080/20787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb Gerrit-Change-Number: 20787 Gerrit-PatchSet: 6 Gerrit-Owner: Ádám Bakai <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai <[email protected]> Gerrit-Comment-Date: Tue, 02 Jan 2024 21:23:08 +0000 Gerrit-HasComments: Yes
