Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/24618 )
Change subject: KUDU-3734 Compaction budget refinements and observability ...................................................................... Patch Set 3: (3 comments) 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: metrics_->compaction_budget_skip_health->set_value(oversized_count > 0 ? 1 : 0); q: The health gauge is set here, but the earlier asc_max_key.empty() short-circuit (line ~526) returns before this. If a tablet was degraded (gauge=1) and later drops to zero candidate rowsets (e.g. the oversized rowset is finally removed), the gauge stays stale at 1. Minor edge case, but worth a thought - should the gauge be reset to 0 on that early return too? 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@456 PS3, Line 456: "Total on-disk size (base + redo + undo) of DiskRowSet candidates not " q: The description says "Total on-disk size (base + redo + undo)", but the value comes from RowSetInfo::base_and_deltas_size_bytes() -> OnDiskBaseDataSizeWithDeltas(), which now multiplies the undo size by --rowset_undo_deltas_budget_weight. So with a weight < 1.0 this counter reports the budget-weighted footprint, not the real on-disk footprint. If the intent is to let operators "gauge the impact / data size being deferred", the weighted value under-reports actual disk usage. Is the weighted value what we want here, and should the description say "weighted undo"? 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: shared_lock l(lock_); This function gauge walks the entire tablet_map_ and dereferences each tablet's metrics on *every* metric scrape. The sibling state-count gauges (tablets_num_running, etc.) deliberately avoid this by caching behind RefreshTabletStateCacheAndReturnCount(), which throttles the full-map walk to at most once per --tablet_state_walk_min_period_ms. On a server with thousands of tablets, a metrics scrape (Prometheus, /metrics) could now trigger a full walk under lock_ on each collection. Should this reuse the same throttled-cache pattern, or at least document why the uncached walk is acceptable here? -- 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: 3 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: Sun, 02 Aug 2026 09:58:57 +0000 Gerrit-HasComments: Yes
