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 <apha...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to