You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "anujphadke (Code Review)" <ge...@cloudera.org> on 2016/09/29 02:50:35 UTC
[Impala-ASF-CR] IMPALA-3342, IMPALA-3920: Adding thread counters to obtain thread stats per scanner thread, and to measure time spent in per-fragment-executor Open() and ProcessBuildInputAsync calls
anujphadke has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4561
Change subject: IMPALA-3342, IMPALA-3920: Adding thread counters to obtain thread stats per scanner thread, and to measure time spent in per-fragment-executor Open() and ProcessBuildInputAsync calls
......................................................................
IMPALA-3342, IMPALA-3920: Adding thread counters to obtain thread stats
per scanner thread, and to measure time spent in per-fragment-executor Open()
and ProcessBuildInputAsync calls
Initial draft. Will add a detailed commit message later.
Change-Id: I1b148eafb76319e727e8d0ef33c49b300b0c887f
(cherry picked from commit 9b9887f1c0479d1c9fedb3eaa44fd30aa327b5ec)
---
M be/src/exec/blocking-join-node.cc
M be/src/exec/blocking-join-node.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/kudu-scan-node.cc
M be/src/exec/kudu-scan-node.h
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
10 files changed, 53 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/4561/1
--
To view, visit http://gerrit.cloudera.org:8080/4561
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1b148eafb76319e727e8d0ef33c49b300b0c887f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-3342, IMPALA-3920: Adding thread counters to obtain thread stats per scanner thread, and to measure time spent in per-fragment-executor Open() and ProcessBuildInputAsync calls
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3342, IMPALA-3920: Adding thread counters to obtain thread stats per scanner thread, and to measure time spent in per-fragment-executor Open() and ProcessBuildInputAsync calls
......................................................................
Patch Set 1:
Is this superseded by https://gerrit.cloudera.org/#/c/4633/?
--
To view, visit http://gerrit.cloudera.org:8080/4561
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1b148eafb76319e727e8d0ef33c49b300b0c887f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3342, IMPALA-3920: Adding thread counters to obtain thread stats per scanner thread, and to measure time spent in per-fragment-executor Open() and ProcessBuildInputAsync calls
Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has abandoned this change.
Change subject: IMPALA-3342, IMPALA-3920: Adding thread counters to obtain thread stats per scanner thread, and to measure time spent in per-fragment-executor Open() and ProcessBuildInputAsync calls
......................................................................
Abandoned
Code review moved here -https://gerrit.cloudera.org/#/c/4633/
--
To view, visit http://gerrit.cloudera.org:8080/4561
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I1b148eafb76319e727e8d0ef33c49b300b0c887f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-3342, IMPALA-3920: Adding thread counters to obtain thread stats per scanner thread, and to measure time spent in per-fragment-executor Open() and ProcessBuildInputAsync calls
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-3342, IMPALA-3920: Adding thread counters to obtain thread stats per scanner thread, and to measure time spent in per-fragment-executor Open() and ProcessBuildInputAsync calls
......................................................................
Patch Set 1:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/4561/1//COMMIT_MSG
Commit Message:
Nit: You might want to set your git user name, so it shows up in your commits.
git config --global user.name "Anuj Phadke"
Line 11: Initial draft. Will add a detailed commit message later.
I couldn't figure out how this addresses IMPALA-3920. Can you explain in the commit message?
http://gerrit.cloudera.org:8080/#/c/4561/1/be/src/exec/blocking-join-node.h
File be/src/exec/blocking-join-node.h:
Line 68: RuntimeProfile::ThreadCounters* process_build_input_async_counters_;
Can this be protected?
http://gerrit.cloudera.org:8080/#/c/4561/1/be/src/exec/hdfs-scan-node.cc
File be/src/exec/hdfs-scan-node.cc:
Line 358: string thread_name = "ScannerThread-" + std::to_string(syscall(SYS_gettid))+ "-";
Something similar is done in L348. Why are the names different? Also, I think we often prefer to use stringstream<< or Substitute() to build strings.
Maybe even better, pass in the name from the caller, similar to the kudu scan node.
http://gerrit.cloudera.org:8080/#/c/4561/1/be/src/exec/hdfs-scan-node.h
File be/src/exec/hdfs-scan-node.h:
Line 25: #include <sys/types.h>
these don't seem used here. Can you move them to the .cc file where they are used?
http://gerrit.cloudera.org:8080/#/c/4561/1/be/src/exec/kudu-scan-node.cc
File be/src/exec/kudu-scan-node.cc:
PS1, Line 274: thread_name
I think you already get the thread name in the 'name' parameter.
Line 275: scanner_thread_counters_ =
I understand that this adds one counter per thread. Couldn't this be a lot of counters and thus lines we add to the profile? Have you considered adding a histogram of counters instead of one per thread?
http://gerrit.cloudera.org:8080/#/c/4561/1/be/src/exec/kudu-scan-node.h
File be/src/exec/kudu-scan-node.h:
Line 24: #include <sys/types.h>
these don't seem used here. Can you move them to the .cc file where they are used?
http://gerrit.cloudera.org:8080/#/c/4561/1/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:
Line 306: plan_fragment_counters_ = ADD_THREAD_COUNTERS(profile(),
Does this have to be a member variable, i.e. does it have to be valid past the scope of this method?
http://gerrit.cloudera.org:8080/#/c/4561/1/be/src/runtime/plan-fragment-executor.h
File be/src/runtime/plan-fragment-executor.h:
Line 150: ///Name of the plan fragment counter
Describe what the counter tracks. The comment and name don't seem to match.
Line 153: ///Fragment thread counters
Can you explain what these count?
Line 154: RuntimeProfile::ThreadCounters* plan_fragment_counters_;
Make this protected?
Line 156: RuntimeProfile::ThreadCounters* plan_fragment_counters() const {
the variable seems to be used internally only, so I don't see why this getter is needed. However, I saw that pattern in other files, too, and if you want to follow it, maybe it's best to also have a setter to initialize the variable. Currently it's set by direct assignment and then referenced with the getter, which I think looks a bit confusing.
--
To view, visit http://gerrit.cloudera.org:8080/4561
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1b148eafb76319e727e8d0ef33c49b300b0c887f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3342, IMPALA-3920: Adding thread counters to obtain thread stats per scanner thread, and to measure time spent in per-fragment-executor Open() and ProcessBuildInputAsync calls
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-3342, IMPALA-3920: Adding thread counters to obtain thread stats per scanner thread, and to measure time spent in per-fragment-executor Open() and ProcessBuildInputAsync calls
......................................................................
Patch Set 1:
(5 comments)
I think we need to refine the design here. I see the main goal as replacing current RuntimeState::total_time() calculation, which is bogus and replacing it with a set of thread counters than includes the time spent in all of the fragment threads.
- I made a few comments but let me know if you want to discuss more offline.
http://gerrit.cloudera.org:8080/#/c/4561/1/be/src/exec/blocking-join-node.h
File be/src/exec/blocking-join-node.h:
Line 68: RuntimeProfile::ThreadCounters* process_build_input_async_counters_;
> Can this be protected?
I think this counter may not be necessary - probably simpler to just count the time in the build thread against the RuntimeProfile's fragment-level counter rather than having the more granular counter (the async build thread will go away with multithreading anyway).
http://gerrit.cloudera.org:8080/#/c/4561/1/be/src/exec/kudu-scan-node.cc
File be/src/exec/kudu-scan-node.cc:
Line 278: SCOPED_THREAD_COUNTER_MEASUREMENT(scanner_thread_agg_counters());
I don't think we really need the thread-level counters, it will make the profile really huge. We do need to include the scanner thread time in the fragment-level counters though.
http://gerrit.cloudera.org:8080/#/c/4561/1/be/src/exec/scan-node.h
File be/src/exec/scan-node.h:
Line 113: RuntimeProfile::ThreadCounters* scanner_thread_agg_counters() const {
How is this different from scanner_thread_counters?
http://gerrit.cloudera.org:8080/#/c/4561/1/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:
Line 277:
Remove extra whitespace
Line 443: int64_t cpu_and_wait_time = fragment_sw_.ElapsedTime();
We want to get rid of this calculation here and replace RuntimeState::total_cpu_timer() with the thread counters (since the current way of computing the cpu time is bogus).
--
To view, visit http://gerrit.cloudera.org:8080/4561
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1b148eafb76319e727e8d0ef33c49b300b0c887f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes