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 <aphadke@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes