Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/24618 )
Change subject: KUDU-3734 Compaction budget refinements and observability ...................................................................... Patch Set 6: (5 comments) Thank you for the review. I have addressed all the comments in PS6. Please take a look. http://gerrit.cloudera.org:8080/#/c/24618/3/src/kudu/tablet/compaction_policy.cc File src/kudu/tablet/compaction_policy.cc: http://gerrit.cloudera.org:8080/#/c/24618/3/src/kudu/tablet/compaction_policy.cc@544 PS3, Line 544: // never be selected for compaction under the current settings. Report via > q: The health gauge is set here, but the earlier asc_max_key.empty() Yes, that is quite possible especially if compaction budget is increased or rowset is GC'ed. Thanks for pointing this out. Resetting the gauge in early-return path, should solve such cases. http://gerrit.cloudera.org:8080/#/c/24618/2/src/kudu/tablet/tablet_metrics.cc File src/kudu/tablet/tablet_metrics.cc: http://gerrit.cloudera.org:8080/#/c/24618/2/src/kudu/tablet/tablet_metrics.cc@447 PS2, Line 447: ange (always expected for large rowset sets) " : "or because the knapsack budget was exhaus > > a persistent rising rate together with a high average_diskrowset_height o Good catch! The idea is to show the rising rate and not a monotonically increasing number counter. Changed to gauge that resets to 0 after each run. If it has a persistent non-zero value along with an increasing 'average_diskrowset_height', operator would be able to confirm that there is at least one rowset that is being skipped for compaction. http://gerrit.cloudera.org:8080/#/c/24618/3/src/kudu/tablet/tablet_metrics.cc File src/kudu/tablet/tablet_metrics.cc: http://gerrit.cloudera.org:8080/#/c/24618/3/src/kudu/tablet/tablet_metrics.cc@446 PS3, Line 446: . Candida > nit: this part seems to be stale after the recent update in PS3 Done http://gerrit.cloudera.org:8080/#/c/24618/3/src/kudu/tablet/tablet_metrics.cc@456 PS3, Line 456: METRIC_DEFINE_gauge_int64(tablet, compact_rs_bytes_budget_skipped, > q: The description says "Total on-disk size (base + redo + undo)", but the Good point! Although changing just the description should be harmless but it could lead to confusion wherever OnDiskBaseDataSizeWithDeltas is used. So, sticking to the OnDiskBaseDataSizeWithDeltas definition as name suggests and let callers make use of that information as per their needs would make more sense. http://gerrit.cloudera.org:8080/#/c/24618/3/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/24618/3/src/kudu/tserver/ts_tablet_manager.cc@417 PS3, Line 417: // if modified by user) is not desirable. > This function gauge walks the entire tablet_map_ and dereferences each Having a cache would introduce staleness which is not desirable for a 'tablets_num_compaction_budget_degraded' type of metric that is more of an alert to the operator that tablet/s are degraded for compaction budget adequacy. Also, I don't see much gain with caching just one gauge as against tablet state cache that is caching eight different gauges. Added a couple of lines to explain the reasoning behind choosing full walk instead of cached-walk. -- To view, visit http://gerrit.cloudera.org:8080/24618 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id63db0006c77ad1d55eb2119d99ee64499435819 Gerrit-Change-Number: 24618 Gerrit-PatchSet: 6 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Mon, 10 Aug 2026 11:56:33 +0000 Gerrit-HasComments: Yes
