impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <ger...@cloudera.org>
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:

(14 comments)

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.

http://gerrit.cloudera.org:8080/#/c/2578/3/be/src/runtime/backend-client.h
File be/src/runtime/backend-client.h:

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


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>
prot)
is this space needed?


Line 30:   ImpalaBackendClient(boost::shared_ptr< ::apache::thrift::protocol::TProtocol>
iprot,
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
time."


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.
transmission


http://gerrit.cloudera.org:8080/#/c/2578/3/be/src/runtime/client-cache.h
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.)


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

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.


http://gerrit.cloudera.org:8080/#/c/2578/3/be/src/util/stopwatch.h
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
basis.


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 http://gerrit.cloudera.org:8080/2578
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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