impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <>
Subject [Impala-CR](cdh5-trunk) IMPALA-2076: Correct execution time tracking for DataStreamSender.
Date Wed, 13 Apr 2016 23:36:27 GMT
Henry Robinson has posted comments on this change.

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

Patch Set 3:


First pass, as I know you want to make progress with this.

Could you add some unit tests for the ConcurrentStopWatch (and maybe the timer) class? You
could start a SW from several threads, let them run for ~1s and then check the elapsed time
is still approximately 1s.
File be/src/runtime/backend-client.h:

Line 15: #include "runtime/client-cache.h"

Line 22: /// Backend Client that can take a RuntimeProfile::ConcurrentTimerCounter to track
       : /// transmit data time for multiple threads. If more than one threads send data
       : /// at the same time, the transmit time is count only once.
Let's make this a bit more clear, e.g.:

"Proxy class that extends ImpalaInternalServiceClient to allow callers to time the wall-clock
time taken in TransmitData(), so that the time spent sending data between backends in a query
can be measured."

Line 27:   ImpalaBackendClient(boost::shared_ptr< ::apache::thrift::protocol::TProtocol>
is this space needed?

Line 30:   ImpalaBackendClient(boost::shared_ptr< ::apache::thrift::protocol::TProtocol>
blank line before this one

Line 45: Caller who wants to track data transmit time need to set its own counter.
Maybe: "Callers of TransmitData() should provide their own counter to measure the data transmission

Line 51:   /// ImpalaBackendClient is shared by multiple queries. It's caller's responsibility
It's the caller's...

Line 52:   /// to reset the counter after data transmition.
File be/src/runtime/client-cache.h:

Line 350: class ImpalaBackendClient;
        : typedef ClientCache<ImpalaBackendClient> ImpalaBackendClientCache;
        : typedef ClientConnection<ImpalaBackendClient> ImpalaBackendConnection;
Let's move these to the header that defines ImpalaBackendClient (and then you can keep the
include that I marked as unused.)
File be/src/runtime/

Line 383: ThriftTransmitTime
We use (*) to indicate that a timer is the sum of several counter's wall times. That's not
the case here, so I'd remove the (*).

In fact, let's call it "TransmitDataRPCTime" to be totally clear what it is.
File be/src/util/stopwatch.h:

Line 187: last_running_time_(0),
        :       busy_threads_(0) {
        :   }
fit this more onto one line

Line 190: ~ConcurrentStopWatch() {
        :     msw_.Stop();
        :   }
I don't think you need this, because ConcurrentStopWatch isn't supposed to be used on a per-scope

Line 214: LastRunningTime
I think we would usually call this the LapTime() (like on a regular stopwatch).

Line 226: /// This is for caller who just want the most recent running time..
Can you define what this is more clearly?

Line 233:   boost::mutex thread_counter_lock_;
I think this is a case where a SpinLock would make some sense, since the critical sections
do no allocation and are extremely short.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I9c530b2056d852c8bcac6263e9e6b1a6bede1047
Gerrit-PatchSet: 3
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