Philip Zeyliger 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: (6 comments) I think this is very close. I think we could add an end-to-end test to make sure the pause monitor is working; what do you think? Otherwise, very minor stuff left as far as I'm concerned. http://gerrit.cloudera.org:8080/#/c/10998/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10998/6//COMMIT_MSG@68 PS6, Line 68: - Tested it manually with kill -SIGSTOP/-SIGCONT <pid>. Made sure that : the non-GC JVM pauses are logged. This test could be automated into our suite easily enough. http://gerrit.cloudera.org:8080/#/c/10998/6/be/src/util/jni-util.cc File be/src/util/jni-util.cc: http://gerrit.cloudera.org:8080/#/c/10998/6/be/src/util/jni-util.cc@214 PS6, Line 214: if (!jni_util_cl_) return Status ("JniUtil::Init() not called."); please remove the space after Status here. 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@173 PS5, Line 173: : if (extraSleepTime > warnThresholdMs_) { : LOG.warn(formatMessage( : extraSleepTime, gcTimesAfterSleep, gcTimesBeforeSleep)); : } else if (extraSleepTime > infoThresholdMs_) { : LOG.info(formatMessage( : extraSleepTime, gcTimesAfterSleep, gcTimesBeforeSleep)); : } > Yep, this came up in the comments so far. We can do a histogram of pauses a Ok. Testing can be made a little bit easier by having a counter for how many times this happens, but I'm fine either way. http://gerrit.cloudera.org:8080/#/c/10998/6/fe/src/test/java/org/apache/impala/util/JniUtilTest.java File fe/src/test/java/org/apache/impala/util/JniUtilTest.java: http://gerrit.cloudera.org:8080/#/c/10998/6/fe/src/test/java/org/apache/impala/util/JniUtilTest.java@55 PS6, Line 55: // Validates the JSON string returned by JniUtil.getJMXJson() Could you move this to JmxJsonUtilTest and test JmxJsonUtil.getJmxJson() directly? i.e., I'm not at all worried about the wrapper function, and it seems like we should be testing the thing that returns a String. http://gerrit.cloudera.org:8080/#/c/10998/6/fe/src/test/java/org/apache/impala/util/JniUtilTest.java@71 PS6, Line 71: assertNotNull("Invalid JSON: " + jmxJson, rootNode.findValue("beans")); For completeness, we should test that it gets the beans we expect. Something like the following is good enough, where 'output' is the JSON string. assertTrue(output.contains("java.lang:type=Memory")); assertTrue(output.contains("java.lang:type=Runtime")); http://gerrit.cloudera.org:8080/#/c/10998/6/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/10998/6/tests/webserver/test_web_pages.py@73 PS6, Line 73: response = requests.get(input_url) Can you see about the content-type that we're getting back here? -- 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: Tue, 31 Jul 2018 18:57:03 +0000 Gerrit-HasComments: Yes
