impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Juan Yu (Code Review)" <>
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:

File be/src/runtime/

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.
File be/src/util/

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

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

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

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

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

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
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?
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I9c530b2056d852c8bcac6263e9e6b1a6bede1047
Gerrit-PatchSet: 4
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Juan Yu <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-HasComments: Yes

View raw message