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 <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
