Henry Robinson has posted comments on this change. Change subject: IMPALA-2076: Correct execution time for DataStreamSender. ......................................................................
Patch Set 2: (1 comment) I think this might be better implemented by doing the following: 1. Add a ConcurrentWallClockTimer class that encapsulates the acquire / release semantics from UpdateSenderStats() 2. Subclass ImpalaInternalServiceClient so that TransmitData() is instrumented by this new counter type (you probably have to add a SetTransmitDataCounter() method as well). I'm worried that adding DoRpcResponse / DoRpcRequest duplicates a lot of the tricky error handling that we have in DoRpc(), and I'm not sure those new methods would get called much. Separately, can you add the justification for the change to the commit message - why the receive time is ignored? http://gerrit.cloudera.org:8080/#/c/2578/2/be/src/runtime/data-stream-sender.h File be/src/runtime/data-stream-sender.h: Line 102: /// Update sender sending timers, count sending time if any channel is sending data. : void UpdateSendingStats(bool busy_channel); I would split this into two methods and get rid of the parameter. Also, "StartSenderTimer()" and "StopSenderTimer()" or "StartBusyChannel()" etc. would be a better name. -- 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: 2 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
