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:
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
- I made a few comments but let me know if you want to discuss more offline.
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).
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.
Line 113: RuntimeProfile::ThreadCounters* scanner_thread_agg_counters() const
How is this different from scanner_thread_counters?
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-Owner: anujphadke <apha...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>