Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/22253 )
Change subject: KUDU-613: Make segment metrics more relevant ...................................................................... Patch Set 2: (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 protect Good point, I'll add usage back. Its the only metric that takes into account the charge of the entries. Inserts/evictions could be misleading since its values are independent of the entry's charge. http://gerrit.cloudera.org:8080/#/c/22253/1/src/kudu/util/cache_metrics.h@49 PS1, Line 49: scoped_refptr<Histogram> upgrades_stats; : scoped_refptr<Histogram> downgrades_stats; > nit: I'd think those are rather summaries on upgrades/downgrades, at least Done 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 introd In most cases it would be straightforward to deduce the value given the new fields. But in the case the fields are of equal value, it's not straightforward. There's two possible scenarios. Scenario 1: When handles are updated in the protected segment, a new handle is inserted. If it gets downgrades then upgraded again, downgrades and upgrades would be equal (both 1) and the handle would be in the protected segment. Scenario 2: In the more common case where a handle is inserted into the probationary segment, and it gets upgraded then downgraded again, the values would be the same (1) but the handle is in the probationary segment. 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 o Upgrades and downgrades are 64-bit since the Histogram::Increment() function takes in a 64-bit value. I can make the lookups parameter 64-bit since that's more likely to be larger than upgrades/downgrades anyways. -- 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: 2 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: Mon, 06 Jan 2025 20:05:31 +0000 Gerrit-HasComments: Yes
