You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org> on 2017/02/15 06:49:20 UTC
[Impala-ASF-CR] IMPALA-4885: Expose Jvm thread info in web UI
Dimitris Tsirogiannis has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6013
Change subject: IMPALA-4885: Expose Jvm thread info in web UI
......................................................................
IMPALA-4885: Expose Jvm thread info in web UI
This commit exposes information about JVM threads to the impalad and
catalogd web UIs. This information includes statistics about the number
of threads running in the JVM as well as per-thread stacktraces, monitors and
synchronizers. Total CPU, user CPU and blocked time is also reported per
thread.
Change-Id: Id497043ab33dcf107a562f0b1ccd5c46095d397f
---
M be/src/catalog/catalogd-main.cc
M be/src/service/impalad-main.cc
M be/src/statestore/statestored-main.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/thread.cc
M be/src/util/thread.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/common/JniUtil.java
A www/jvm-threadz.tmpl
M www/threadz.tmpl
11 files changed, 312 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/6013/1
--
To view, visit http://gerrit.cloudera.org:8080/6013
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id497043ab33dcf107a562f0b1ccd5c46095d397f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
[Impala-ASF-CR] IMPALA-4885: Expose Jvm thread info in web UI
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
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 <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4885: Expose Jvm thread info in web UI
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Hello Henry Robinson,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6013
to look at the new patch set (#4).
Change subject: IMPALA-4885: Expose Jvm thread info in web UI
......................................................................
IMPALA-4885: Expose Jvm thread info in web UI
This commit exposes information about JVM threads to the impalad and
catalogd web UIs. This information includes statistics about the number
of threads running in the JVM as well as per-thread stacktraces, monitors and
synchronizers. Total CPU, user CPU and blocked time is also reported per
thread.
Change-Id: Id497043ab33dcf107a562f0b1ccd5c46095d397f
---
M be/src/catalog/catalogd-main.cc
M be/src/service/impalad-main.cc
M be/src/statestore/statestored-main.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/thread.cc
M be/src/util/thread.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/common/JniUtil.java
A www/jvm-threadz.tmpl
M www/threadz.tmpl
A www/threadz_tabs.tmpl
12 files changed, 402 insertions(+), 78 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/6013/4
--
To view, visit http://gerrit.cloudera.org:8080/6013
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id497043ab33dcf107a562f0b1ccd5c46095d397f
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-4885: Expose Jvm thread info in web UI
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-4885: Expose Jvm thread info in web UI
......................................................................
Patch Set 1:
A screenshot of the new web page with the JVM thread info is here:
https://cloudera.box.com/s/agu113d1rxzfainy860f6oqlizp4v92v
--
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 <dt...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4885: Expose Jvm thread info in web UI
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4885: Expose Jvm thread info in web UI
......................................................................
Patch Set 4: Verified+1
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4885: Expose Jvm thread info in web UI
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-4885: Expose Jvm thread info in web UI
......................................................................
Patch Set 4: Code-Review+2
Rebase and keep Henry's +2
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4885: Expose Jvm thread info in web UI
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-4885: Expose Jvm thread info in web UI
......................................................................
IMPALA-4885: Expose Jvm thread info in web UI
This commit exposes information about JVM threads to the impalad and
catalogd web UIs. This information includes statistics about the number
of threads running in the JVM as well as per-thread stacktraces, monitors and
synchronizers. Total CPU, user CPU and blocked time is also reported per
thread.
Change-Id: Id497043ab33dcf107a562f0b1ccd5c46095d397f
Reviewed-on: http://gerrit.cloudera.org:8080/6013
Reviewed-by: Dimitris Tsirogiannis <dt...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/catalog/catalogd-main.cc
M be/src/service/impalad-main.cc
M be/src/statestore/statestored-main.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/thread.cc
M be/src/util/thread.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/common/JniUtil.java
A www/jvm-threadz.tmpl
M www/threadz.tmpl
A www/threadz_tabs.tmpl
12 files changed, 402 insertions(+), 78 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Dimitris Tsirogiannis: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/6013
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Id497043ab33dcf107a562f0b1ccd5c46095d397f
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-4885: Expose Jvm thread info in web UI
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-4885: Expose Jvm thread info in web UI
......................................................................
Patch Set 1:
A publicly available screenshot:
https://drive.google.com/file/d/0B7VW0hRyzVlKUGdWakNIdlhtbTQ/view?usp=sharing
--
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 <dt...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4885: Expose Jvm thread info in web UI
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
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 <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4885: Expose Jvm thread info in web UI
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has uploaded a new patch set (#3).
Change subject: IMPALA-4885: Expose Jvm thread info in web UI
......................................................................
IMPALA-4885: Expose Jvm thread info in web UI
This commit exposes information about JVM threads to the impalad and
catalogd web UIs. This information includes statistics about the number
of threads running in the JVM as well as per-thread stacktraces, monitors and
synchronizers. Total CPU, user CPU and blocked time is also reported per
thread.
Change-Id: Id497043ab33dcf107a562f0b1ccd5c46095d397f
---
M be/src/catalog/catalogd-main.cc
M be/src/service/impalad-main.cc
M be/src/statestore/statestored-main.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/thread.cc
M be/src/util/thread.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/common/JniUtil.java
A www/jvm-threadz.tmpl
M www/threadz.tmpl
A www/threadz_tabs.tmpl
12 files changed, 404 insertions(+), 78 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/6013/3
--
To view, visit http://gerrit.cloudera.org:8080/6013
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id497043ab33dcf107a562f0b1ccd5c46095d397f
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-4885: Expose Jvm thread info in web UI
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4885: Expose Jvm thread info in web UI
......................................................................
Patch Set 1:
(5 comments)
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 jsons? I think the one right above it now corresponds to ThreadGroupUrlCallback()? No?
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.
PS1, Line 327: INFO
warn/error? How about bubbling this error up to the web endpoint by setting the "error" member? (Other places too).
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?
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 blocked on? (ThreadInfo#getLockInfo() / getLockOwnerId() etc.).. Helps debugging hangs.
--
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 <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4885: Expose Jvm thread info in web UI
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has uploaded a new patch set (#2).
Change subject: IMPALA-4885: Expose Jvm thread info in web UI
......................................................................
IMPALA-4885: Expose Jvm thread info in web UI
This commit exposes information about JVM threads to the impalad and
catalogd web UIs. This information includes statistics about the number
of threads running in the JVM as well as per-thread stacktraces, monitors and
synchronizers. Total CPU, user CPU and blocked time is also reported per
thread.
Change-Id: Id497043ab33dcf107a562f0b1ccd5c46095d397f
---
M be/src/catalog/catalogd-main.cc
M be/src/service/impalad-main.cc
M be/src/statestore/statestored-main.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/thread.cc
M be/src/util/thread.h
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/common/JniUtil.java
A www/jvm-threadz.tmpl
M www/threadz.tmpl
A www/threadz_tabs.tmpl
12 files changed, 401 insertions(+), 77 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/6013/2
--
To view, visit http://gerrit.cloudera.org:8080/6013
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id497043ab33dcf107a562f0b1ccd5c46095d397f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-4885: Expose Jvm thread info in web UI
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-4885: Expose Jvm thread info in web UI
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/6013/3/be/src/util/thread.cc
File be/src/util/thread.cc:
PS3, Line 79: args
> long line
Done
PS3, Line 340: void InitThreading() {
: DCHECK(thread_manager.get() == nullptr);
: thread_manager.reset(new ThreadMgr());
: }
:
: Status StartThreadInstrumentation(MetricGroup* metrics, Webserver* webserver,
: bool include_jvm_threads) {
: DCHECK(metrics != nullptr);
: DCHECK(webserver != nullptr);
: RETURN_IF_ERROR(thread_manager->StartInstrumentation(metrics));
: RegisterUrlCallbacks(include_jvm_threads, webserver);
: return Status::OK();
: }
> If you move these to the end of the file, does that avoid any of the need t
Yes, we don't need to forward declare RegisterUrlCallbacks. Done
PS3, Line 379: args
> long line
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4885: Expose Jvm thread info in web UI
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4885: Expose Jvm thread info in web UI
......................................................................
Patch Set 1:
This is super useful. Could you post the screenshot somewhere that's publicly accessible?
--
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 <dt...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4885: Expose Jvm thread info in web UI
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4885: Expose Jvm thread info in web UI
......................................................................
Patch Set 4:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/352/
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4885: Expose Jvm thread info in web UI
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
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 <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4885: Expose Jvm thread info in web UI
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-4885: Expose Jvm thread info in web UI
......................................................................
Patch Set 3:
Any more comments on this? Henry, lmk if you're swamped and I will ask someone else to take a look.
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4885: Expose Jvm thread info in web UI
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4885: Expose Jvm thread info in web UI
......................................................................
Patch Set 1:
(5 comments)
this is pretty cool. Got a couple of suggestions about organisation and presentation, let me know what you think.
http://gerrit.cloudera.org:8080/#/c/6013/1/be/src/util/thread.cc
File be/src/util/thread.cc:
PS1, Line 209: RegisterUrlCallback
Rather than adding another top-level link, consider making two tabs on the threadz.tmpl page - maybe thread overview and JVM threads for now. See query_detail_tabs.tmpl for how to add tabs to a page in bootstrap.
PS1, Line 234: bool track_jvm_threads,
how about just making this a member of ThreadMgr? Actually - I don't think ThreadMgr needs to know about JVM threads at all, right? Why not just have something like:
void ThreadOverviewUrlCallback(...) {
thread_mgr->GetThreadOverview(document);
GetJvmThreadOverview(document);
}
and then you avoid having to pass track_jvm_threads to ThreadMgr at all, which is good because it doesn't really track or manage those threads.
Line 260: << status.GetDetail();
return, then remove else { }
PS1, Line 262: jvmThreadsVal
C++ naming styles
http://gerrit.cloudera.org:8080/#/c/6013/1/www/jvm-threadz.tmpl
File www/jvm-threadz.tmpl:
PS1, Line 35: Is n
Native
--
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 <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4885: Expose Jvm thread info in web UI
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4885: Expose Jvm thread info in web UI
......................................................................
Patch Set 3: Code-Review+2
(3 comments)
http://gerrit.cloudera.org:8080/#/c/6013/3/be/src/util/thread.cc
File be/src/util/thread.cc:
PS3, Line 79: args
long line
PS3, Line 340: void InitThreading() {
: DCHECK(thread_manager.get() == nullptr);
: thread_manager.reset(new ThreadMgr());
: }
:
: Status StartThreadInstrumentation(MetricGroup* metrics, Webserver* webserver,
: bool include_jvm_threads) {
: DCHECK(metrics != nullptr);
: DCHECK(webserver != nullptr);
: RETURN_IF_ERROR(thread_manager->StartInstrumentation(metrics));
: RegisterUrlCallbacks(include_jvm_threads, webserver);
: return Status::OK();
: }
If you move these to the end of the file, does that avoid any of the need to forward declare RegisterUrlCallbacks() and ThreadOverviewUrlCallback()?
PS3, Line 379: args
long line
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes