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: (3 comments) http://gerrit.cloudera.org:8080/#/c/22253/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22253/2//COMMIT_MSG@11 PS2, Line 11: frequency nit: stats 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; > In most cases it would be straightforward to deduce the value given the new Thanks for elaborating on this. I think this exposes an inconsistency in how upgrades and downgrades are counted per entry as of PS2. If the implementation of the cache replaces and entry for a particular key and the entry is still staying in the same segment, semantically the upgrade/downgrade counters shouldn't change, right? But contrary to that, the counters are now zeroed out. That leads to completely skewed stats on upgrades and downgrades, doesn't it? What do you think? http://gerrit.cloudera.org:8080/#/c/22253/1/src/kudu/util/slru_cache.h@69 PS1, Line 69: int64_t > Upgrades and downgrades are 64-bit since the Histogram::Increment() functio Could int32_t be implicitly converted without any issues into int64_t when supplied into Histogram::Increment()? If so, then it would make more sense to make upgrades and downgrades 32-bit counters as well, otherwise it's more bytes wasted per entry. Or you expect to see the number of look-ups/upgrades/downgrades per entry going beyond 2^31? -- 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: Fri, 10 Jan 2025 20:07:35 +0000 Gerrit-HasComments: Yes
