impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [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
Date Thu, 29 Sep 2016 15:59:00 GMT
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

Mime
View raw message