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 3: (1 comment) 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: static int64_t GetSerializedBatchSize( : const TransmitDataRequestPB* request, RpcContext* rpc_context) { : kudu::Slice tuple_offsets; : if (UNLIKELY(!rpc_context->GetInboundSidecar( : request->tuple_offsets_sidecar_idx(), &tuple_offsets).ok())) { : return 0; : } : kudu::Slice tuple_data; : if (UNLIKELY(!rpc_context->GetInboundSidecar( : request->tuple_data_sidecar_idx(), &tuple_data).ok())) { : return 0; : } : return tuple_data.size() + tuple_offsets.size(); : } > Convert to a static function. UnpackRequest() was returning deserialized si Right, but it also gives the serialized size components. What I meant was: Status status = UnpackRequest(request, rpc_context, &tuple_offset, &uple_data, ¬_used); return status.ok() ? tuple_data.size() + tuple_offsets.size() : 0; okay to ignore, but was just thinking it'd be good if things can't get out of sync by sharing code (for example, if another sidecar were to be added). -- 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: 3 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 18:50:22 +0000 Gerrit-HasComments: Yes
