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 <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Juan Yu <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
