Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20758 )
Change subject: [compaction] Fix the incorrect memory budgeting condition ...................................................................... Patch Set 2: > > The patch looks good. > > However, I have a general comment about this part of the code: I > > executed the whole diskrowset-test.cc and since > > FLAGS_rowset_compaction_memory_estimate_enabled > > is set to false during test, the code is never executed during > > tests. I think thats the reason that it wasn't discovered during > > testing. If I set FLAGS_rowset_compaction_memory_estimate_enabled > > to true, then the function throws an error because it tries to > > downcast MockDiskRowset to DiskRowset, so it's not > straightforward. > > I'm working on a patch that at introduces a new test where this > > function is executed. > > Yes, that and other budgeting size conditions that are specific to > certain scenarios like frequent upserts resulting in accumulation > of UNDO deltas. Unless the test introduces such conditions, chances > are that budgeting policy won't apply. > > I am not sure what error it is throwing in your case but I would be > happy to discuss if you are stuck and need help understanding this > part of code. The problem comes from the nature of mock diskrowsets used in those tests. IIUC, to have a proper test for this, it's necessary either to introduce a condition with real DiskRowSets (i.e. something based on ExternalMiniCluster), or create a test scaffolding with mock rowsets that based on a class sub-classing (a.k.a. inheriting from) DiskRowSet. The former has stuck in limbo, and it's been in that state for about one year: https://gerrit.cloudera.org/#/c/19278/ The latter would be a new approach, and it might be a feasible one if also mocking memory usage or, alternatively, somehow setting the parameters for the thresholds based on the real memory usage during test runtime to exercise the condition of lacking memory to run merge rowset compaction. Given that the first approach didn't materialize as an actual patch, maybe pursuing the latter would be a better option. As one can see, this bug just emphasizes the necessity of adding test coverage for every piece of new functionality introduced. -- To view, visit http://gerrit.cloudera.org:8080/20758 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8928b15750f100785c510ee8086e5a6281b3a7b8 Gerrit-Change-Number: 20758 Gerrit-PatchSet: 2 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Ádám Bakai <[email protected]> Gerrit-Comment-Date: Fri, 08 Dec 2023 20:46:24 +0000 Gerrit-HasComments: No
