Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9690 )
Change subject: IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender ...................................................................... Patch Set 5: (13 comments) http://gerrit.cloudera.org:8080/#/c/9690/3/be/src/runtime/krpc-data-stream-recvr.cc File be/src/runtime/krpc-data-stream-recvr.cc: http://gerrit.cloudera.org:8080/#/c/9690/3/be/src/runtime/krpc-data-stream-recvr.cc@457 PS3, Line 457: kudu::Slice tuple_offsets; : kudu::Slice tuple_data; : int64_t batch_size; : status = UnpackRequest(ctx->request, ctx->rpc_context, &tuple_offsets, : &tuple_data, &batch_size); : // Reply with error status if the entry cannot be unpacked. : if (UNLIKELY(!status.ok())) { : DataStreamService::RespondAndReleaseRpc(status, ctx->response, ctx->rpc_context, : recvr_->deferred_rpc_tracker()); : DequeueDeferredRpc(); : return; : } : : > Right, but it also gives the serialized size components. What I meant was: Done 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: doe > does Done http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@121 PS4, Line 121: doe > does Done http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@207 PS4, Line 207: l_has_deferred_rpcs_timer_'. > how about calling it: has_deferred_rpcs_start_time_ns_? Done http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@318 PS4, Line 318: deferred_rpcs_.pop(); > DCHECK_NE(has_deferred_rpcs_start_time_ns, 0)? Done http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@320 PS4, Line 320: ed_rpcs_start_time_ns_, 0 > how about calling this: total_has_deferred_rpcs_time_ to make it clearer th Done http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@642 PS4, Line 642: chWork() > sometimes we have "counter_" and sometimes not. Let's try to be consistent Done http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@646 PS4, Line 646: rious > okay to ignore, but I think it'd be good to call these timer_ (but calling Done http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@673 PS4, Line 673: total_deferred_rpcs_count > as mentioned earlier, how about total_has_deferred_rpcs_time_ to distinguis Done http://gerrit.cloudera.org:8080/#/c/9690/4/be/src/runtime/krpc-data-stream-recvr.cc@674 PS4, Line 674: "TotalRPCsDeferred", > TotalHasDeferredRPCsTime Done 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: m > maybe add: but not meaningful by itself, and therefore not placed in a prof Done 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: Substitute("TransmitData() to $0 failed", TNetworkAddressToString(address_)); > maybe set rpc_start_time_ns_ to 0 here and then DCHECK at that it's not 0 a Set to 0 in MarkDone(). 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: receivin > let's clarify that, since it could be intepretted as the data stream recvr Done -- 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: 5 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: Tue, 27 Mar 2018 00:46:22 +0000 Gerrit-HasComments: Yes
