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 5: (6 comments) Remaining comments are mainly about maintainability. 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: long GetCounterMetric(std::function<int64_t(TGetJvmMemoryMetricsResponse&)> accessor); I believe the general best practice for passing functions to lightweight functions like this is to make this a template function with the function type as a template parameter. The issue is that there's overhead in converting a lambda to a std::function, because std::function has to have a uniform representation for passing in arbitrary numbers of captured variables. https://vittorioromeo.info/index/blog/passing_functions_to_functions.html has some explanation. http://gerrit.cloudera.org:8080/#/c/11468/5/be/src/util/memory-metrics.h@181 PS5, Line 181: static const int64_t kCachePeriodMillis = 1000; I guess the google style guide says to use this naming convention https://google.github.io/styleguide/cppguide.html#Constant_Names, but we've pretty consistently used UPPER_CASE for constants in C++. http://gerrit.cloudera.org:8080/#/c/11468/5/be/src/util/memory-metrics.h@224 PS5, Line 224: std::function<int64_t(TGetJvmMemoryMetricsResponse&)> accessor); I guess we already materialise it to a std::function here so maybe my above comment doesn't make a difference, but maybe we should pass it by const reference above to avoid copying it? We could make it templates all the way down by making this class templated by the function type, but that feels like overkill. http://gerrit.cloudera.org:8080/#/c/11468/5/fe/src/main/java/org/apache/impala/util/JvmPauseMonitor.java File fe/src/main/java/org/apache/impala/util/JvmPauseMonitor.java: http://gerrit.cloudera.org:8080/#/c/11468/5/fe/src/main/java/org/apache/impala/util/JvmPauseMonitor.java@62 PS5, Line 62: // Overall metrics Could be worth mentioning why they're volatile, because it's an unusual and somewhat risky pattern if misapplied. E.g. "Volatile to allow populating metrics concurrently with the values being updated without staleness (but with no other synchronization guarantees)." http://gerrit.cloudera.org:8080/#/c/11468/5/tests/infra/test_jvm_metrics.py File tests/infra/test_jvm_metrics.py: http://gerrit.cloudera.org:8080/#/c/11468/5/tests/infra/test_jvm_metrics.py@21 PS5, Line 21: class TestJvmMetrics(ImpalaTestSuite): I'm not sure that infra/ is the right place for this test, since I think it means "test infra". It seems to fit with webserver/test_web_pages.py. Maybe we should consolidate into an observability/ subdirectory, since that seems to be the general theme. http://gerrit.cloudera.org:8080/#/c/11468/5/tests/infra/test_jvm_metrics.py@29 PS5, Line 29: METRICS = [ NON_ZERO_METRICS? -- 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: 5 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 05:56:51 +0000 Gerrit-HasComments: Yes
