Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-4885: Expose Jvm thread info in web UI ......................................................................
Patch Set 1: (10 comments) Thanks for the suggestions Henry. New screenshots are here: https://drive.google.com/file/d/0B7VW0hRyzVlKTzZPR3VEVlVEYWc/view?usp=sharing https://drive.google.com/file/d/0B7VW0hRyzVlKOXowTUM2QWVySm8/view?usp=sharing 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 j No, this is the output of the overview callback. 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. This has changed. PS1, Line 209: RegisterUrlCallback > Rather than adding another top-level link, consider making two tabs on the Done PS1, Line 234: bool track_jvm_threads, > how about just making this a member of ThreadMgr? Actually - I don't think Done Line 260: << status.GetDetail(); > return, then remove else { } Done PS1, Line 262: jvmThreadsVal > C++ naming styles power of habit.. :) Done PS1, Line 327: INFO > warn/error? How about bubbling this error up to the web endpoint by setting Changed it to WARN. What is the "error" member? 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? Done 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 It already does that. http://gerrit.cloudera.org:8080/#/c/6013/1/www/jvm-threadz.tmpl File www/jvm-threadz.tmpl: PS1, Line 35: Is n > Native Done -- 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
