Bharath Vissapragada 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: (13 comments) 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@167 PS5, Line 167: caching gathering fix this? http://gerrit.cloudera.org:8080/#/c/11468/5/be/src/util/memory-metrics.h@169 PS5, Line 169: class JvmMetricCache { Add some method docs and some details about locking http://gerrit.cloudera.org:8080/#/c/11468/5/be/src/util/memory-metrics.h@217 PS5, Line 217: class JvmMemoryCounterMetric : public IntCounter { docs. http://gerrit.cloudera.org:8080/#/c/11468/5/be/src/util/memory-metrics.cc File be/src/util/memory-metrics.cc: http://gerrit.cloudera.org:8080/#/c/11468/5/be/src/util/memory-metrics.cc@241 PS5, Line 241: if (now - last_fetch_ < kCachePeriodMillis) { : return; : } nit: single line http://gerrit.cloudera.org:8080/#/c/11468/5/be/src/util/memory-metrics.cc@252 PS5, Line 252: boost:: I think 'using' declarations are preferred in the source file rather than qualifying it everywhere. http://gerrit.cloudera.org:8080/#/c/11468/5/be/src/util/memory-metrics.cc@290 PS5, Line 290: void JvmMetricCache::GetPoolNames(vector<string>& names) { nit: Either return a vector<> or make it a ptr? https://google.github.io/styleguide/cppguide.html#Output_Parameters http://gerrit.cloudera.org:8080/#/c/11468/5/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/11468/5/common/thrift/Frontend.thrift@769 PS5, Line 769: struct TGetJvmMemoryMetricsRequest { Why keep the empty one? http://gerrit.cloudera.org:8080/#/c/11468/5/fe/src/main/java/org/apache/impala/common/JniUtil.java File fe/src/main/java/org/apache/impala/common/JniUtil.java: http://gerrit.cloudera.org:8080/#/c/11468/5/fe/src/main/java/org/apache/impala/common/JniUtil.java@132 PS5, Line 132: TGetJvmMemoryMetricsRequest request = new TGetJvmMemoryMetricsRequest(); : JniUtil.deserializeThrift(protocolFactory_, request, argument); Not needed anymore? http://gerrit.cloudera.org:8080/#/c/11468/5/fe/src/main/java/org/apache/impala/common/JniUtil.java@200 PS5, Line 200: jvmMetrics.setGc_num_warn_threshold_exceeded( Commented elsewhere, these are not always GCs AFAICT. If we are only interested in num pauses, may be rename it to num_pause_warn_threshold...? http://gerrit.cloudera.org:8080/#/c/11468/5/fe/src/main/java/org/apache/impala/common/JniUtil.java@210 PS5, Line 210: for (GarbageCollectorMXBean bean : ManagementFactory.getGarbageCollectorMXBeans()) { : gcCount += bean.getCollectionCount(); : gcTimeMillis += bean.getCollectionTime(); : } I'm not super sure if it makes sense to add these across different GarbageCollectors. I think we are more interested in specific Garbage collectors. Also qq, these are the counts (and elapsed times) aggregated till this point in time right? From a debugging POV, doesn't it make sense to poll the last GC information (like in JMX output)? Because we are usually interested in in certain windows when it happens. 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@201 PS5, Line 201: numGcWarnThresholdExceeded Per my understanding, a pause is not always a GC pause (look at formatMessage()). should you distinguish between these two pauses or rename it to numPause.... http://gerrit.cloudera.org:8080/#/c/11468/5/tests/custom_cluster/test_pause_monitor.py File tests/custom_cluster/test_pause_monitor.py: http://gerrit.cloudera.org:8080/#/c/11468/5/tests/custom_cluster/test_pause_monitor.py@36 PS5, Line 36: # Wait for a few seconds for the logs to get flushed. : time.sleep(5) still needed? 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@22 PS5, Line 22: """Class for pause monitor tests.""" Doesn't look like it? -- 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 15:49:29 +0000 Gerrit-HasComments: Yes
