Michael Ho has posted comments on this change. Change subject: IMPALA-4856: Port data stream service to KRPC ......................................................................
Patch Set 3: (6 comments) Some more comments. Still going through the patch. http://gerrit.cloudera.org:8080/#/c/7103/3/be/src/rpc/rpc.h File be/src/rpc/rpc.h: PS3, Line 106: Ownership is : // shared by the caller, and the RPC subsystem Doesn't std::move transfer the ownership so the caller no longer shares the ownership, right ? PS3, Line 143: are owned by the caller the ownership is temporarily transferred to the RPC call when this function is invoked, right ? http://gerrit.cloudera.org:8080/#/c/7103/3/be/src/runtime/data-stream-sender.cc File be/src/runtime/data-stream-sender.cc: PS3, Line 214: !channel channel == nullptr PS3, Line 252: batch->compressed_tuple_data Is this transferring the ownership to the RPC subsystem ? AddSideCar() internally uses std::move(). This seems subtle enough to warrant a comment. PS3, Line 266: MonoDelta::FromMilliseconds(numeric_limits<int32_t>::max()) This is a subtle change in behavior from previous Impala version. In particular, FLAGS_backend_client_rpc_timeout_ms marks that the timeout for a socket if a thrift thread was stuck writing to the socket. Given KRPC socket is asynchronous, the DSS may get blocked for quite a while until the query gets cancelled. Should we impose some reasonably conservative timeout here ? http://gerrit.cloudera.org:8080/#/c/7103/3/be/src/runtime/row-batch.cc File be/src/runtime/row-batch.cc: PS3, Line 117: DCHECK( DCHECK_EQ -- To view, visit http://gerrit.cloudera.org:8080/7103 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia66704be7a0a8162bb85556d07b583ec756c584b Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-HasComments: Yes
