Philip Zeyliger 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 6: (19 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 the gathe > fix this? Done http://gerrit.cloudera.org:8080/#/c/11468/5/be/src/util/memory-metrics.h@169 PS5, Line 169: // > Add some method docs and some details about locking Done http://gerrit.cloudera.org:8080/#/c/11468/5/be/src/util/memory-metrics.h@172 PS5, Line 172: // lock_. > I believe the general best practice for passing functions to lightweight fu I replaced std::function with the specific function pointer type. Here, the type is always the same (int64_t)(*f)(const TGetJvmMemorymetricsResponse&), which just rolls off the tongue, eh? I figure being specific is going to be easier to deal with than templating the class, since we need to store this thing too. I also chose not to typedef this because something in the Google style guide says you shouldn't expose typedefs in your API. Anyway--let me know. http://gerrit.cloudera.org:8080/#/c/11468/5/be/src/util/memory-metrics.h@181 PS5, Line 181: static JvmMetricCache* GetInstance(); > I guess the google style guide says to use this naming convention https://g Done http://gerrit.cloudera.org:8080/#/c/11468/5/be/src/util/memory-metrics.h@217 PS5, Line 217: > docs. Done http://gerrit.cloudera.org:8080/#/c/11468/5/be/src/util/memory-metrics.h@224 PS5, Line 224: }; > I guess we already materialise it to a std::function here so maybe my above Right now it's a function pointer, so I think the concern is moot. Let me know if I'm wrong. 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_ < CACHE_PERIOD_MILLIS) return; : if (!JniUtil::GetJvmMemoryMetrics(&last_response_).ok()) return; : l > nit: single line Done http://gerrit.cloudera.org:8080/#/c/11468/5/be/src/util/memory-metrics.cc@252 PS5, Line 252: > I think 'using' declarations are preferred in the source file rather than q Done http://gerrit.cloudera.org:8080/#/c/11468/5/be/src/util/memory-metrics.cc@290 PS5, Line 290: vector<string> names; > nit: Either return a vector<> or make it a ptr? Done 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: > Why keep the empty one? Old habits about believing that Thrift should be compatible. Doesn't apply here, so I've removed it throughout. 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: jvmMetrics.setMemory_pools(new ArrayList<TJvmMemoryPool>()); : TJvmMemoryPool totalUsage = new TJvmMemoryPool(); > Not needed anymore? Done http://gerrit.cloudera.org:8080/#/c/11468/5/fe/src/main/java/org/apache/impala/common/JniUtil.java@200 PS5, Line 200: jvmMetrics.setGc_total_extra_sleep_time_millis( > Commented elsewhere, these are not always GCs AFAICT. If we are only intere I'm trying to keep the metric names consistent with Hadoop. http://gerrit.cloudera.org:8080/#/c/11468/5/fe/src/main/java/org/apache/impala/common/JniUtil.java@210 PS5, Line 210: jvmMetrics.setGc_count(gcCount); : jvmMetrics.setGc_time_millis(gcTimeMillis); : : r > I'm not super sure if it makes sense to add these across different GarbageC A few answers here. This is consistent with Hadoop, and that's nice, since tooling for one can work for the other. As much as anything, an overall count of the seconds spent on garbage collection is an ok proxy for how much you're paying for GC. If you're spending 1000 millis per second on GC, you've pinned a thread to GC, and that's typically bad. The gccount stuff is less defensible, but when correlated with memory usage charts, you can see what's going on. We already expose the specific GC metrics via /jmx. i.e., if you wanted to separate by collector, you could. Generically, though, the collector you're using depends on your Java flags and on your JVM, so a monitoring tool might want something more generic that's always available. Anyway: on balance, I think consistency with Hadoop is reasonable. I'm open to removing gcCount. I think gcTimeMillis is useful. For your other question, these are counters. If you want to understand what's happening over a time period, you get those counters into a time series, and the deltas tell you what's going on. 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 Done http://gerrit.cloudera.org:8080/#/c/11468/5/fe/src/main/java/org/apache/impala/util/JvmPauseMonitor.java@201 PS5, Line 201: String, GcTimes> gcTimesAf > Per my understanding, a pause is not always a GC pause (look at formatMessa Your understanding is right: all we know about is pauses. In this case, however, I think it's better to be consistent with the original (https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/JvmPauseMonitor.java) rather than diverge. 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 over a second for the cache metrics to expire. : time.sleep(1. > still needed? Done 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: > I'm not sure that infra/ is the right place for this test, since I think it Created observability. I'll double-check that it gets executed there, but I think I added it to the right place. http://gerrit.cloudera.org:8080/#/c/11468/5/tests/infra/test_jvm_metrics.py@22 PS5, Line 22: > Doesn't look like it? Done http://gerrit.cloudera.org:8080/#/c/11468/5/tests/infra/test_jvm_metrics.py@29 PS5, Line 29: > NON_ZERO_METRICS? Done -- 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: 6 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:33:33 +0000 Gerrit-HasComments: Yes
