impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4885: Expose Jvm thread info in web UI
Date Sat, 25 Feb 2017 01:31:57 GMT
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 <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message