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 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/23348/9/src/kudu/tablet/rowset_info.cc File src/kudu/tablet/rowset_info.cc: http://gerrit.cloudera.org:8080/#/c/23348/9/src/kudu/tablet/rowset_info.cc@475 PS9, Line 475: if (FLAGS_rowset_merge_compaction_weight_includes_undo_deltas) { : extra_->base_and_deltas_size_bytes = rs->OnDiskBaseDataSizeWithDeltas(); : } else { : extra_->base_and_deltas_size_bytes = rs->OnDiskBaseDataSizeWithRedos(); : } The idea was to encapsulate the logic into the RowSet::OnDiskBaseDataSizeWithDeltas() implementation. In earlier revisions (e.g., at least in PS6), there is already a version of RowSet that behaved unconditionally different by changing the logic and renaming OnDiskBaseDataSizeWithRedos() into OnDiskBaseDataSizeWithDeltas(). It might be a good venue to introduce a flag that would control the behavior, making it configurable. The RowSetInfo's logic is downstream from the one that RowSet has, and that's great to have no exposure to that logic in the RowSetInfo's implementation. Aside from hypothetical considerations that OnDiskBaseDataSizeWithRedos() might be needed in future, it seems there isn't any other reason to keep OnDiskBaseDataSizeWithRedos() and OnDiskBaseDataSizeWithDeltas() as two separate methods in the current code, right? If so, then according to the YAGNI principle https://martinfowler.com/bliki/Yagni.html it's better to keep just OnDiskBaseDataSizeWithDeltas(). Having this observations in mind, what do you think of renaming 'rowset_merge_compaction_weight_includes_undo_deltas' into 'rowset_deltas_size_include_undo' and encapsulating the logic of including the size of undo deltas into the OnDiskBaseDataSizeWithDeltas() implementation? > Additionally, this if/else logic based on the flag is present at only two > places: here and in compaction_policy-test.c (where its purpose is to come up > with a total size value that doesn't exceed budget limit). What about tablet.cc? IIUC, the UB/SIGSEGV condition which could have been introduced in PS8 (https://gerrit.cloudera.org/#/c/23348/8/src/kudu/tablet/tablet.cc@2248) appeared in an attempt to duplicate the logic outside of the RowSet class. -- 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: 9 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, 25 Feb 2026 21:36:08 +0000 Gerrit-HasComments: Yes
