Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-4885: Expose Jvm thread info in web UI ......................................................................
Patch Set 2: (11 comments) 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 symbo Done PS2, Line 216: NULL > nullptr, here and everywhere Done PS2, Line 379: DCHECK(document != NULL); > why would document be null? Good point. Removed from here and elsewhere. PS2, Line 384: GetJvmThreadOverview > move this into ThreadOverviewUrlCallback Done 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 Done PS2, Line 393: << status.GetDetail(); > formatting Done 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 ? Done 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? Done 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()) ? Done 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 ? opera Hm. Doesn't work. For some reason it doesn't populate it when I replace it with the ? operator. http://gerrit.cloudera.org:8080/#/c/6013/2/www/threadz_tabs.tmpl File www/threadz_tabs.tmpl: Line 27: </ul> > nit: trailing whitespace 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: 2 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
