Henry Robinson has posted comments on this change. Change subject: IMPALA-4885: Expose Jvm thread info in web UI ......................................................................
Patch Set 2: (11 comments) Just minor things. http://gerrit.cloudera.org:8080/#/c/6013/2/be/src/util/thread.cc File be/src/util/thread.cc: PS2, Line 75: void JvmThreadsUrlCallback(const Webserver::ArgumentMap& args, Document* doc); : : void ThreadOverviewUrlCallback(bool track_jvm_threads, const Webserver::ArgumentMap& args, : Document* document); : : void RegisterUrlCallbacks(bool track_jvm_threads, Webserver* webserver); : : void GetJvmThreadOverview(Document* document); put all these in an anonymous namespace to avoid polluting the global symbol table. PS2, Line 216: NULL nullptr, here and everywhere PS2, Line 379: DCHECK(document != NULL); why would document be null? PS2, Line 384: GetJvmThreadOverview move this into ThreadOverviewUrlCallback PS2, Line 392: LOG(WARNING) << "Couldn't retrieve information about JVM threads: " : << status.GetDetail(); But - put this in the "error" member of document, and it should show on the webpage automatically. All the templates can print an error message if one exists. PS2, Line 393: << status.GetDetail(); formatting http://gerrit.cloudera.org:8080/#/c/6013/2/be/src/util/thread.h File be/src/util/thread.h: PS2, Line 194: track_jvm_threads include_jvm_threads ? http://gerrit.cloudera.org:8080/#/c/6013/2/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: Line 758: // Information about JVM threads comment when this might not be set? http://gerrit.cloudera.org:8080/#/c/6013/2/fe/src/main/java/org/apache/impala/common/JniUtil.java File fe/src/main/java/org/apache/impala/common/JniUtil.java: PS2, Line 206: threadCount just write response.setTotal_thread_count(threadBean.getThreadCount()) ? http://gerrit.cloudera.org:8080/#/c/6013/2/www/threadz.tmpl File www/threadz.tmpl: PS2, Line 37: # nit: is there only one jvm-threads entry? If so, this should be the ? operator (rather than the for-each operator). http://gerrit.cloudera.org:8080/#/c/6013/2/www/threadz_tabs.tmpl File www/threadz_tabs.tmpl: Line 27: </ul> nit: trailing whitespace -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-HasComments: Yes
