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: (9 comments) http://gerrit.cloudera.org:8080/#/c/10998/4/be/src/util/default-path-handlers.h File be/src/util/default-path-handlers.h: http://gerrit.cloudera.org:8080/#/c/10998/4/be/src/util/default-path-handlers.h@34 PS4, Line 34: MetricGroup* metric_group = NULL); > why did you make the jmx path optional here? Is this because the statestore Yea, its a common code path for all of them. Looks like getJNIEnv() creates a JVM if one doesn't exist. I cleaned this up to add a flag in JniUtil which tells if a JVM exists. http://gerrit.cloudera.org:8080/#/c/10998/4/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/4/fe/src/main/java/org/apache/impala/util/JMXJsonUtil.java@71 PS4, Line 71: public class JMXJsonUtil { > I didn't review this file particularly since it's just cross-ported. Any pa Nothing much. Looks straightforward. I commented a couple of refactors that I did incase you want to diff with the upstream version http://gerrit.cloudera.org:8080/#/c/10998/4/fe/src/main/java/org/apache/impala/util/JMXJsonUtil.java@79 PS4, Line 79: public static String getJMXJson() { Removed a bunch of servlet code. https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/jmx/JMXJsonServlet.java#L170 http://gerrit.cloudera.org:8080/#/c/10998/4/fe/src/main/java/org/apache/impala/util/JMXJsonUtil.java@89 PS4, Line 89: Removed the 'qry' functionality. https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/jmx/JMXJsonServlet.java#L206 http://gerrit.cloudera.org:8080/#/c/10998/4/fe/src/main/java/org/apache/impala/util/JMXJsonUtil.java@198 PS4, Line 198: trace Moved this from upstream debug() to trace() since the default logging in Impala fe is debug(). This was firing for a few attributes and was dumping a bunch of stacks and looks like for the same reason it was made debug in the Hadoop code. https://github.com/apache/hadoop/commit/2d1406e9e7b75a833f79c0159031dae2ba3e6134 http://gerrit.cloudera.org:8080/#/c/10998/4/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/4/fe/src/main/java/org/apache/impala/util/JvmPauseMonitor.java@42 PS4, Line 42: public class JvmPauseMonitor { > same here, any changes worth noting to take a look at? Commented the refactoring I did. Nothing major. http://gerrit.cloudera.org:8080/#/c/10998/4/fe/src/main/java/org/apache/impala/util/JvmPauseMonitor.java@42 PS4, Line 42: { Removed dependency on AbstractService http://gerrit.cloudera.org:8080/#/c/10998/4/fe/src/main/java/org/apache/impala/util/JvmPauseMonitor.java@69 PS4, Line 69: private JvmPauseMonitor() { : this(INFO_THRESHOLD_MS, WARN_THRESHOLD_MS); : } Not relying on Hadoop Conf objects. http://gerrit.cloudera.org:8080/#/c/10998/4/fe/src/main/java/org/apache/impala/util/JvmPauseMonitor.java@160 PS4, Line 160: Stopwatch Uses Stopwatch from Guava. -- 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 <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Fri, 27 Jul 2018 23:46:07 +0000 Gerrit-HasComments: Yes
