Juan Yu has posted comments on this change. Change subject: IMPALA-2076: Correct execution time tracking for DataStreamSender. ......................................................................
Patch Set 3: (14 comments) 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? Defined ImpalaBackendClient in this header and keep this. 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.: Done Line 27: ImpalaBackendClient(boost::shared_ptr< ::apache::thrift::protocol::TProtocol> prot) > is this space needed? Thanks for pointing it out. yes, it's needed. without space, compiler will complain. In file included from /home/cdh52/Impala/be/src/runtime/exec-env.h:26: /home/cdh52/Impala/be/src/runtime/backend-client.h:30:40: error: found '<::' after a template name which forms the digraph '<:' (aka '[') and a ':', did you mean '< ::'? ImpalaBackendClient(boost::shared_ptr<::apache::thrift::protocol::TProtocol> prot) ^~~ < :: Line 30: ImpalaBackendClient(boost::shared_ptr< ::apache::thrift::protocol::TProtocol> iprot, > blank line before this one Done 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 measu Done Line 51: /// ImpalaBackendClient is shared by multiple queries. It's caller's responsibility > It's the caller's... Done Line 52: /// to reset the counter after data transmition. > transmission Done 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 y Done 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 ti Done 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 Done Line 190: ~ConcurrentStopWatch() { : msw_.Stop(); : } > I don't think you need this, because ConcurrentStopWatch isn't supposed to Done Line 214: LastRunningTime > I think we would usually call this the LapTime() (like on a regular stopwat Done Line 226: /// This is for caller who just want the most recent running time.. > Can you define what this is more clearly? Done Line 233: boost::mutex thread_counter_lock_; > I think this is a case where a SpinLock would make some sense, since the cr Done -- 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 <j...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: Juan Yu <j...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-HasComments: Yes
