Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11468 )
Change subject: IMPALA-7596. Adding JvmPauseMonitor (and other GC) metrics to Impala metrics. ...................................................................... Patch Set 7: Code-Review+1 (3 comments) http://gerrit.cloudera.org:8080/#/c/11468/6/be/src/util/memory-metrics.h File be/src/util/memory-metrics.h: http://gerrit.cloudera.org:8080/#/c/11468/6/be/src/util/memory-metrics.h@240 PS6, Line 240: I don't think we want the space before & http://gerrit.cloudera.org:8080/#/c/11468/5/be/src/util/memory-metrics.h File be/src/util/memory-metrics.h: http://gerrit.cloudera.org:8080/#/c/11468/5/be/src/util/memory-metrics.h@172 PS5, Line 172: // lock_. > I replaced std::function with the specific function pointer type. Here, the WFM. I think a typedef would be fine since this is an internal API (I suspect at Google the concerns are related to APIs that are consumed by many dependencies). I think this is exposing the fact that I don't know all the corners of the google style guide that well :) http://gerrit.cloudera.org:8080/#/c/11468/5/be/src/util/memory-metrics.h@181 PS5, Line 181: static JvmMetricCache* GetInstance(); > Done Just to complete the circle, I updated our list of differences from the google style guide to mention this case. -- To view, visit http://gerrit.cloudera.org:8080/11468 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia707393962ad94ef715ec015b3fe3bb1769104a2 Gerrit-Change-Number: 11468 Gerrit-PatchSet: 7 Gerrit-Owner: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 26 Sep 2018 18:41:41 +0000 Gerrit-HasComments: Yes
