Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23348 )
Change subject: KUDU-3734 include undo size while picking rowsets ...................................................................... Patch Set 6: Code-Review+1 (3 comments) Looks good to me from the code-related perspective. However, there are a few very important follow-up action items here: 0. Should there be a flag to switch between the former and the new rowset merge compaction budgeting approach? In the items below there is more context on why it might be a good idea. 1. It's necessary to re-evaluate the default setting for the --tablet_compaction_budget_mb flag. Since the accumulated delta history might be quite large (by current default the system keeps 7 days of delta history) and the progress in minor/major delta compactions does not help with reducing the the rowset estimated 'size' once the UNDO deltas are taking into account, I suspect that the updated policy could seize up already existing systems in production, so no rowset merge compaction happening at all. 2. On the related note, it's necessary to test how the compaction behaves on real workloads with this updated policy. Is there a risk of the compaction activity falling behind and eventually not compacting a single rowset because of long tail of accumulated UNDO delta history? 3. How does this affect small rowset compaction (see KUDU-1400)? Should we expect any surprises once this update deployed on systems running real world workloads? As a realistic workload, consider playing with YCSB or similar. Just running 'kudu perf loadgen ... --num_rows_per_thread=-1 ...' for a day or two with UPSERT enabled might be another workload to validate this new policy. You might find useful information on running YCSB workloads against a Kudu cluster in changelist b915df7f335ec947cfc8339aa8f59be0188e4469. There are many other changelists in the git history mentioning YCSB and related experiments that were performed when making similar changes in the past. http://gerrit.cloudera.org:8080/#/c/23348/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23348/3//COMMIT_MSG@10 PS3, Line 10: taking into consideration the size of undo deltas while picking rowsets : during rowset compaction. I could not find any historical reason to why : it was not done before. Maybe there was some analysis done that ended : in a conclusion that considering undo deltas may not be right approach : when estimating upper bound fractional solution in the knapsack. > This algorithm is only applicable to rowset merge compaction. I don't see t I took a closer look at the compaction code and it turned out you were right: the knapsack solver is used only for picking rowsets for merge compaction (essentially, it's usage is limited to BudgetedCompactionPolicy::PickRowSets()). After some more spelunking, from the logs in the git repo I found that some time ago other people were also puzzled by only redo deltas were considered when assessing the size of a rowset: e.g. take a look at the comment of changelist 3e3bd1ccbc2b4b070c733b36b1971de63977428b With that, it's not clear to me why undo deltas weren't originally accounted for, either. http://gerrit.cloudera.org:8080/#/c/23348/3//COMMIT_MSG@38 PS3, Line 38: [ ] RowSet(217)( 1M) [0.0027, 0.1953] > Basically, what is happening here is that rowset 217 is ranging from 0.0027 Thanks a lot for the explanation; that makes sense, indeed. http://gerrit.cloudera.org:8080/#/c/23348/6//COMMIT_MSG Commit Message: PS6: Do you mind adding a test to track regressions if anything changes and the compaction starts picking up rowsets beyond the budget again? It's OK with me if doing that in a separate changelist. The crucial point is making sure the fixed/updated behavior doesn't regress back to the original if changing the related code in future. Thank you! -- To view, visit http://gerrit.cloudera.org:8080/23348 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I351c0ba96a02e6ded5153adf9d981083a8c40592 Gerrit-Change-Number: 23348 Gerrit-PatchSet: 6 Gerrit-Owner: Ashwani Raina <[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: Kurt Deschler <[email protected]> Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Wed, 28 Jan 2026 21:21:31 +0000 Gerrit-HasComments: Yes
