Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/10998 )
Change subject: IMPALA-6857: Add Jvm pause/GC Monitor utility and expose JMX metrics ...................................................................... Patch Set 6: (13 comments) http://gerrit.cloudera.org:8080/#/c/10998/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10998/5//COMMIT_MSG@46 PS5, Line 46: now emmitted > nit: *now* emitted, right? Ouch, you are right. http://gerrit.cloudera.org:8080/#/c/10998/5/be/src/util/default-path-handlers.cc File be/src/util/default-path-handlers.cc: http://gerrit.cloudera.org:8080/#/c/10998/5/be/src/util/default-path-handlers.cc@199 PS5, Line 199: Status status = JniUtil::GetJMXJson(&result); > is it worth adding something like: Done http://gerrit.cloudera.org:8080/#/c/10998/5/be/src/util/default-path-handlers.cc@199 PS5, Line 199: Status status = JniUtil::GetJMXJson(&result); > Also, let's log on error here, at least once? Done http://gerrit.cloudera.org:8080/#/c/10998/5/be/src/util/default-path-handlers.cc@243 PS5, Line 243: > Is raw_text_pre.tmpl missing from this review? Ouch, I was experimenting with various template files and settled for common-pre.tmpl. Forgot to git-add. Coming to the HTML question, you can pass "?raw" param to /jmx to fetch raw text contents (example in commit message). http://gerrit.cloudera.org:8080/#/c/10998/5/be/src/util/jni-util.h File be/src/util/jni-util.h: http://gerrit.cloudera.org:8080/#/c/10998/5/be/src/util/jni-util.h@295 PS5, Line 295: /// Gets JMX metrics of the JVM encoded as a JSON string. > "encoded as a JSON string"? Done http://gerrit.cloudera.org:8080/#/c/10998/5/be/src/util/jni-util.cc File be/src/util/jni-util.cc: http://gerrit.cloudera.org:8080/#/c/10998/5/be/src/util/jni-util.cc@214 PS5, Line 214: if (!jni_util_cl_) return Status ("JniUtil::Init() not called."); > Does this fail clang-tidy because of the space after Status? Probably shoul No it didn't. I'm not too familiar with it, so not sure why. Fixed it anyway. http://gerrit.cloudera.org:8080/#/c/10998/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/10998/5/fe/src/main/java/org/apache/impala/common/JniUtil.java@102 PS5, Line 102: byte[] serializeToThrift(T input, F protocolFactory) throws ImpalaException { > This is boring, but please add a unit test for it; should be easy. There is an open jira to try out more efficient protocols but I'm not able to locate it. So, I'm just leaving the code as is to make it easy for that change. Lemme know if you feel strongly about the renaming and I can do it. Added the test. http://gerrit.cloudera.org:8080/#/c/10998/5/fe/src/main/java/org/apache/impala/common/JniUtil.java@211 PS5, Line 211: return serializeToThrift(jvmMetrics, protocolFactory_); > nit: You shouldn't need "JniUtil." here because you're already in the conte Done http://gerrit.cloudera.org:8080/#/c/10998/5/fe/src/main/java/org/apache/impala/util/JMXJsonUtil.java File fe/src/main/java/org/apache/impala/util/JMXJsonUtil.java: http://gerrit.cloudera.org:8080/#/c/10998/5/fe/src/main/java/org/apache/impala/util/JMXJsonUtil.java@72 PS5, Line 72: // MBean server instance > instance Done http://gerrit.cloudera.org:8080/#/c/10998/5/fe/src/main/java/org/apache/impala/util/JMXJsonUtil.java@73 PS5, Line 73: protected static transient MBeanServer mBeanServer = > why is this transient? I haven't put much thought into this, but looking at the hadoop code, I think that is because the JsonFactory implements a serializable interface and we don't want to serialize/persist the entire MBeanServer instance maybe? http://gerrit.cloudera.org:8080/#/c/10998/5/fe/src/main/java/org/apache/impala/util/JMXJsonUtil.java@151 PS5, Line 151: Removed. http://gerrit.cloudera.org:8080/#/c/10998/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/10998/5/fe/src/main/java/org/apache/impala/util/JvmPauseMonitor.java@60 PS5, Line 60: instance > nit: typo Done http://gerrit.cloudera.org:8080/#/c/10998/5/fe/src/main/java/org/apache/impala/util/JvmPauseMonitor.java@173 PS5, Line 173: : if (extraSleepTime > warnThresholdMs_) { : LOG.warn(formatMessage( : extraSleepTime, gcTimesAfterSleep, gcTimesBeforeSleep)); : } else if (extraSleepTime > infoThresholdMs_) { : LOG.info(formatMessage( : extraSleepTime, gcTimesAfterSleep, gcTimesBeforeSleep)); : } > Perhaps we should add Impala metrics for number of detected pauses? (Please Yep, this came up in the comments so far. We can do a histogram of pauses and expose metrics like - Last 3 pauses (GC/non-GC) - Max/min and other %le metrics For ex: Count: 3, min / max: 98ms / 125ms, 25th %-ile: 98ms, 50th %-ile: 102ms, 75th %-ile: 102ms, 90th %-ile: 125ms, 95th %-ile: 125ms, 99.9th %-ile: 125ms Will file a jira to do it as a follow-up patch. -- To view, visit http://gerrit.cloudera.org:8080/10998 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I30d897b7e063846ad6d8f88243e2c04264da0341 Gerrit-Change-Number: 10998 Gerrit-PatchSet: 6 Gerrit-Owner: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Mon, 30 Jul 2018 22:55:04 +0000 Gerrit-HasComments: Yes
