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 5: (6 comments) 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. Done 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. aah, how did I miss it. Done. 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)); : } > Ok. Testing can be made a little bit easier by having a counter for how man I implemented the log-search based approach, but ofcourse once we add these metrics, we can confirm that they are updated too. 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: > Could you move this to JmxJsonUtilTest and test JmxJsonUtil.getJmxJson() di Done http://gerrit.cloudera.org:8080/#/c/10998/6/fe/src/test/java/org/apache/impala/util/JniUtilTest.java@71 PS6, Line 71: > For completeness, we should test that it gets the beans we expect. Somethin Done 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: assert response.status_code == requests.codes.ok\ > Can you see about the content-type that we're getting back here? Done -- 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: 5 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: Wed, 01 Aug 2018 01:23:24 +0000 Gerrit-HasComments: Yes
