Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21389 )
Change subject: KUDU-613: Add SLRU Cache metrics ...................................................................... Patch Set 11: (6 comments) Almost there. Just one nit and a question w.r.t. making SLRUCacheShart a template instead of adding an extra boolean field. http://gerrit.cloudera.org:8080/#/c/21389/11//COMMIT_MSG Commit Message: PS11: > This patch is still before the block cache integration patch. I just rebase Ah, I see. OK, this should be good then. It seems I'm a bit lost with the ordering of the patches -- thanks for the clarification. http://gerrit.cloudera.org:8080/#/c/21389/11/src/kudu/util/block_cache_metrics.cc File src/kudu/util/block_cache_metrics.cc: PS11: > The constructor needs to be implemented for it to be used in the tests. Ah, it seems I'm a bit lost at PS11 then :) http://gerrit.cloudera.org:8080/#/c/21389/11/src/kudu/util/cache_metrics.h File src/kudu/util/cache_metrics.h: http://gerrit.cloudera.org:8080/#/c/21389/11/src/kudu/util/cache_metrics.h@65 PS11, Line 65: scoped_refptr<AtomicGauge<uint64_t>> protected_segment_cache_usage; > This patch already includes those as well. The struct 'SLRUCacheMetrics' in Whoops, it seems I went to many cycles of re-hydrating the context of this patch. My bad -- indeed, the required information it's already present. >From the other side, maybe an alternative of providing the generic metrics via >functional gauges that source the information from the probational and >protected segments might help to simplify the logic, but that's already >outside of the context of this patch. By any means, 11 revisions for this >patch seems to be a lot -- thanks for working through this! http://gerrit.cloudera.org:8080/#/c/21389/11/src/kudu/util/slru_cache.h File src/kudu/util/slru_cache.h: http://gerrit.cloudera.org:8080/#/c/21389/11/src/kudu/util/slru_cache.h@193 PS11, Line 193: bool probationary_; Instead of adding this as a member field, did you consider making this a parameter for the SLRUHandle class template? It might result in a bit cleaner code (template specialization instead of if/else), and might have better performance characteristics. http://gerrit.cloudera.org:8080/#/c/21389/11/src/kudu/util/slru_cache.cc File src/kudu/util/slru_cache.cc: http://gerrit.cloudera.org:8080/#/c/21389/11/src/kudu/util/slru_cache.cc@380 PS11, Line 380: <decltype(mutex_)> just FYI nit (no need to address this, especially if you aren't excited revving the patch just because of this): it's possible omit this part, at least with C++17 http://gerrit.cloudera.org:8080/#/c/21389/11/src/kudu/util/slru_cache.cc@567 PS11, Line 567: shard_pair->SetMetrics(dynamic_cast<SLRUCacheMetrics*>(metrics_.get())); nit: does it make sense to add a DCHECK() for dynamic_cast<SLRUCacheMetrics*>(metrics_.get()) to spot mistakes in the code earlier? -- To view, visit http://gerrit.cloudera.org:8080/21389 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1c8181ec6bea301605aaef7db0003c3eaef3072d Gerrit-Change-Number: 21389 Gerrit-PatchSet: 11 Gerrit-Owner: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Comment-Date: Tue, 30 Jul 2024 00:55:11 +0000 Gerrit-HasComments: Yes
