Ádám Bakai has posted comments on this change. ( http://gerrit.cloudera.org:8080/20787 )
Change subject: [compaction] Add memory estimation unit test ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/20787/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20787/1//COMMIT_MSG@7 PS1, Line 7: Add memory estimation unit test > Maybe add more details about how changing hard limit helps cover test scena Added comment in the testcase itself. // Set memory limit to a specified number to make sure that limit is always hit // during the test, and it won't depend on actual system memory limit. http://gerrit.cloudera.org:8080/#/c/20787/1//COMMIT_MSG@9 PS1, Line 9: FLAGS_rowset_compaction_estimate_min_deltas_size_mb > Did you mean rowset_compaction_memory_estimate_enabled? Done http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy-test.cc File src/kudu/tablet/compaction_policy-test.cc: http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy-test.cc@649 PS1, Line 649: FLAGS_rowset_compaction_memory_estimate_enabled > Isn't this disabled by default? No need to set it to false here unless it c The idea was that what if it gets enabled by default later? Then the test will fail. Maybe it's better to rewrite the test at that time. If you think it's better to rewrite I can remove it. http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy.cc File src/kudu/tablet/compaction_policy.cc: http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy.cc@182 PS1, Line 182: { > Move above to align with other instances. Done http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/rowset.cc File src/kudu/tablet/rowset.cc: http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/rowset.cc@266 PS1, Line 266: for (const shared_ptr<RowSet> &rs : new_rowsets_) > OnDiskDeltaSize used in compaction is per rowset. Here it seems to be compu It's not getting used yet. It was created to follow the pattern in DuplicatingRowSet, where the the returned value of rowsets from new_rowsets_ are added up and returned. -- 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: 1 Gerrit-Owner: Ádám Bakai <aba...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Ashwani Raina <ara...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai <aba...@cloudera.com> Gerrit-Comment-Date: Thu, 14 Dec 2023 11:28:17 +0000 Gerrit-HasComments: Yes
