impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Juan Yu (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-2076: Correct execution time tracking for DataStreamSender.
Date Fri, 15 Apr 2016 06:38:38 GMT
Juan Yu has posted comments on this change.

Change subject: IMPALA-2076: Correct execution time tracking for DataStreamSender.
......................................................................


Patch Set 4:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/2578/4/be/src/runtime/data-stream-sender.cc
File be/src/runtime/data-stream-sender.cc:

Line 209:   COUNTER_ADD(parent_->profile_->total_time_counter(),
> I'm not totally sure if this does the right thing - presumably you want to 
I don't want to add the wall-clock time for all sender threads together. The DataStreamSender
total_time = total serialize time + thrift_transmit_timer_. I want to add delta here to get
Total_time updated frequently, the lap time is what I want. I'll cleanup the LapTime() implementation
like you suggested.


http://gerrit.cloudera.org:8080/#/c/2578/4/be/src/util/runtime-profile-test.cc
File be/src/util/runtime-profile-test.cc:

Line 550:       delete thread_handle;
> how does this work if thread_handle is NULL (see line 584)?
Done


Line 557:     while(!worker->done) {
> space before (
Done


Line 577:   void StopWorkers(int index) {
> needs a comment. maybe call 'index' something like 'exclude_idx' and defaul
Done


Line 580: index >= 0
> how would index be < 0?
I'll add comments. StopWorkers() can stop a specific thread, or all threads. -1 means all
threads.


Line 589:   static const int MAX_NUM_THREADS = 5;
> why have this, rather than just initialize workers_[] in StartWorkers?
Removed.


Line 608: void ValidateLapTime(TimerCounterTest& timer, int64_t expected_value) {
> missing const?
This one cannot be const, LapTime() will update ConcurrentStopWatch member variable.


Line 638: int running_time = 0;
> rather than tracking running_time per sleep-period, it might be better to u
Thanks for pointing this out, Done.


Line 659:   tester.StartWorkers(2, 0);
> suggest adding a sleep before this to show that the time between working pe
Done


http://gerrit.cloudera.org:8080/#/c/2578/4/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:

Line 379: If a thread is already running, the following thread won't reset the stop watch.
> this is confusing - what's the following thread?
Done


http://gerrit.cloudera.org:8080/#/c/2578/4/be/src/util/stopwatch.h
File be/src/util/stopwatch.h:

Line 208: /// Returns running time since last time LapTime() is called and reset lap_time_
        :   /// back to 0.
> this doesn't seem to match the implementation. If we call LapTime() twice i
Sorry for the confusion. I do need Lap time, for caller who wants delta update of concurrent
running time. .


-- 
To view, visit http://gerrit.cloudera.org:8080/2578
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9c530b2056d852c8bcac6263e9e6b1a6bede1047
Gerrit-PatchSet: 4
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jyu@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Juan Yu <jyu@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message