Henry Robinson has posted comments on this change. Change subject: IMPALA-4889: Use client sidecars for Thrift RPCs ......................................................................
Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/6473/1/be/src/rpc/rpc.h File be/src/rpc/rpc.h: Line 101: // RPC invocation using Execute(). > why synchronous? This patch doesn't include async RPCs. I'll remove the references. (They're in the next, big patch). Just to explain this case: an async RPC may complete and call its callback after its Rpc object has been destroyed. In that case, the RpcController is passed directly to the async completion callback. Line 102: Status GetInboundSidecar(int idx, kudu::Slice* sidecar) { > 'inbound' sounds a bit misleading, my initial response was 'you mean server Changed to GetResponseSidecar(). Line 169: int idx = -1; > that shouldn't be necessary. I think it's technically undefined behavior not to initialize it here because the compiler can't tell that it gets initialized in AddSidecar(). Line 170: AddSidecar(kudu::Slice(serialized), &idx); > looks like this is making a copy, bummer. is there some way to avoid that? It's only copying the slice (which is basically a ptr + len pair). See documentation for AddSidecar() - the memory backing the slice is not copied. Line 176: RETURN_IF_ERROR(GetInboundSidecar(response_proto.sidecar_idx(), &sidecar)); > do we not want to reserve this mechanism for large pieces of data? I think it works equally well for small and large, and saves the extra copy in both cases. Line 183: Rpc(const Rpc& other) { > comment why needed Removed - needed for the next patch. Line 207: // Rpc controller storage. Used only for synchronous RPCs so that the caller can access > how do we do async rpcs again? They're in the next patch - references to them here are the result of re-ordering the patches. -- To view, visit http://gerrit.cloudera.org:8080/6473 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0fcc87c6b67aa167d70ae022663b14bc90261c95 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
