Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-4885: Expose Jvm thread info in web UI ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/6013/1/be/src/util/thread.cc File be/src/util/thread.cc: Line 178: void ThreadOverviewUrlCallback(bool track_jvm_threads, Shouldn't this go above ThreadGroupUrlCallback() to match with the sample jsons? I think the one right above it now corresponds to ThreadGroupUrlCallback()? No? PS1, Line 194: [this, track_jvm_threads] (const Webserver::ArgumentMap& args, Document* doc) { : this->ThreadOverviewUrlCallback(track_jvm_threads, args, doc) Make this a method? MakeUrlCallBack or something, used multiple times. PS1, Line 327: INFO warn/error? How about bubbling this error up to the web endpoint by setting the "error" member? (Other places too). http://gerrit.cloudera.org:8080/#/c/6013/1/be/src/util/thread.h File be/src/util/thread.h: Line 191: /// the "thread-manager." Describe track_jvm_threads in the method comment? http://gerrit.cloudera.org:8080/#/c/6013/1/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: Line 720: // Summary of a JVM thread. Includes stacktraces, locked monitors Do you think its good to include the locked synchronizers or the lock it is blocked on? (ThreadInfo#getLockInfo() / getLockOwnerId() etc.).. Helps debugging hangs. -- To view, visit http://gerrit.cloudera.org:8080/6013 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id497043ab33dcf107a562f0b1ccd5c46095d397f Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-HasComments: Yes
