[kudu-CR] [compaction] Add memory estimation unit test

["Alexey Serbin (Code Review)" <ge...@cloudera.org>]

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 <ab...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
Gerrit-Comment-Date: Tue, 02 Jan 2024 21:23:08 +0000
Gerrit-HasComments: Yes