Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22253 )
Change subject: KUDU-613: Make segment metrics more relevant ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/22253/1/src/kudu/util/cache_metrics.h File src/kudu/util/cache_metrics.h: http://gerrit.cloudera.org:8080/#/c/22253/1/src/kudu/util/cache_metrics.h@a67 PS1, Line 67: Once the usage gauges are gone, is there a way to understand if the protected segment is under-utilized, so it might make sense making its size smaller but increasing the probational segment's size instead? Or that's enough just to track 'protected_segment_evictions' vs' protected_segment_inserts' and 'upgrades'? http://gerrit.cloudera.org:8080/#/c/22253/1/src/kudu/util/cache_metrics.h@49 PS1, Line 49: scoped_refptr<Histogram> upgrades_frequency; : scoped_refptr<Histogram> downgrades_frequency; nit: I'd think those are rather summaries on upgrades/downgrades, at least in terms of Prometheus [1] and alike; another blanket-style suffix might be just 'stats' :) [1] https://prometheus.io/docs/concepts/metric_types/#summary http://gerrit.cloudera.org:8080/#/c/22253/1/src/kudu/util/slru_cache.h File src/kudu/util/slru_cache.h: http://gerrit.cloudera.org:8080/#/c/22253/1/src/kudu/util/slru_cache.h@65 PS1, Line 65: // True if entry is in protected segment, false if not. : // Used for releasing from the right shard in the SLRU cache implementation. : std::atomic<bool> in_protected_segment; Do we still need this field once the 'upgrades' and 'downgrades' are introduced below? Semantically, it's straightforward to deduce the value of this field given two new fields below, no? http://gerrit.cloudera.org:8080/#/c/22253/1/src/kudu/util/slru_cache.h@69 PS1, Line 69: int64_t 'lookups' is a 32-bit counter, but 'upgrades' and 'downgrades' are 64-bit ones: that's a bit surprising. Maybe, make these 32-bit as well? -- To view, visit http://gerrit.cloudera.org:8080/22253 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id44828be41a19593e6808debf7e9ba8e1fc4dcca Gerrit-Change-Number: 22253 Gerrit-PatchSet: 1 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Comment-Date: Sat, 21 Dec 2024 01:48:35 +0000 Gerrit-HasComments: Yes
