Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9690 )
Change subject: IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender ...................................................................... Patch Set 4: (12 comments) http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc File be/src/runtime/krpc-data-stream-recvr.cc: http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@117 PS4, Line 117: did does http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@121 PS4, Line 121: did does http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@207 PS4, Line 207: deferred_rpcs_start_time_ns_ how about calling it: has_deferred_rpcs_start_time_ns_? http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@318 PS4, Line 318: if (deferred_rpcs_.empty()) { DCHECK_NE(has_deferred_rpcs_start_time_ns, 0)? http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@320 PS4, Line 320: total_deferred_rpcs_time_ how about calling this: total_has_deferred_rpcs_time_ to make it clearer that it's a total of the !isempty() property (as opposed to total over all deferred rpcs). http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@642 PS4, Line 642: counter_ sometimes we have "counter_" and sometimes not. Let's try to be consistent to make it easier to read. http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@646 PS4, Line 646: time_ okay to ignore, but I think it'd be good to call these timer_ (but calling the thing it ultimately aggregates to a "time" e.g. DataWaitTime makes sense to me). Otherwise, there's no way to know they should be used with the timer macros since they are all Counter* types. http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@673 PS4, Line 673: total_deferred_rpcs_time_ as mentioned earlier, how about total_has_deferred_rpcs_time_ to distinguish that this isn't the total time across deferred rpcs, and the fact that it doesn't really relate to TotalRPCsDeferred. http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@674 PS4, Line 674: TotalRPCsDeferralTime TotalHasDeferredRPCsTime http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-sender.h File be/src/runtime/krpc-data-stream-sender.h: http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-sender.h@183 PS4, Line 183: . maybe add: but not meaningful by itself, and therefore not placed in a profile. http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-sender.cc File be/src/runtime/krpc-data-stream-sender.cc: http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-sender.cc@405 PS4, Line 405: } maybe set rpc_start_time_ns_ to 0 here and then DCHECK at that it's not 0 at the beginning of this function, to verify it was always set to a valid start time for this rpc. http://gerrit.cloudera.org:8080/#/c/9690/4/common/protobuf/data_stream_service.proto File common/protobuf/data_stream_service.proto: http://gerrit.cloudera.org:8080/#/c/9690/4/common/protobuf/data_stream_service.proto@53 PS4, Line 53: receiver let's clarify that, since it could be intepretted as the data stream recvr only. Maybe say "receiving daemon" or similar. -- To view, visit http://gerrit.cloudera.org:8080/9690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd Gerrit-Change-Number: 9690 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Comment-Date: Mon, 26 Mar 2018 19:54:05 +0000 Gerrit-HasComments: Yes
