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 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/22253/3/src/kudu/util/cache_metrics.h File src/kudu/util/cache_metrics.h: http://gerrit.cloudera.org:8080/#/c/22253/3/src/kudu/util/cache_metrics.h@45 PS3, Line 45: : scoped_refptr<Counter> upgrades; : scoped_refptr<Counter> downgrades; Since upgrades_stats and downgrades_stats histogram metrics are now present, could we get away with relying on their 'total_count' fields for the total number of upgrades and downgrades instead of having these dedicated counters? http://gerrit.cloudera.org:8080/#/c/22253/3/src/kudu/util/slru_cache.h File src/kudu/util/slru_cache.h: http://gerrit.cloudera.org:8080/#/c/22253/3/src/kudu/util/slru_cache.h@67 PS3, Line 67: std::atomic<bool> in_protected_segment; With the current way of handling the 'upgrades' and 'downgrades', would it make sense to remove this field and compare the values of 'upgrades' and 'downgrades' fields to determine the location of an entry? That would require making 'upgrades' and 'downgrades' atomics, I guess. However, I think there is simpler way to handling this: you just need to store the pointer to metrics in http://gerrit.cloudera.org:8080/#/c/22253/3/src/kudu/util/slru_cache.h@69 PS3, Line 69: int32_t upgrades; Whoops, sorry -- I missed it in the prior review rounds, but it's better late than never :) Consider documenting these, especially given the rules of setting these fields are quite special. I guess it makes to emphasize that these fields preserve the history of an entry traveling between the segments of the SLRU cache, and the history is logically bound to the key, not an instance of entry. -- 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: 3 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: Fri, 10 Jan 2025 23:42:43 +0000 Gerrit-HasComments: Yes
