impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <>
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:


Just minor things.
File be/src/util/

PS2, Line 75: void JvmThreadsUrlCallback(const Webserver::ArgumentMap& args, Document*
            : void ThreadOverviewUrlCallback(bool track_jvm_threads, const Webserver::ArgumentMap&
            :     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();
File be/src/util/thread.h:

PS2, Line 194: track_jvm_threads
include_jvm_threads ?
File common/thrift/Frontend.thrift:

Line 758:   // Information about JVM threads
comment when this might not be set?
File fe/src/main/java/org/apache/impala/common/

PS2, Line 206: threadCount
just write response.setTotal_thread_count(threadBean.getThreadCount()) ?
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).
File www/threadz_tabs.tmpl:

Line 27: </ul> 
nit: trailing whitespace

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Id497043ab33dcf107a562f0b1ccd5c46095d397f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-HasComments: Yes

View raw message