impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4856: Port data stream service to KRPC
Date Mon, 19 Jun 2017 21:55:53 GMT
Michael Ho has posted comments on this change.

Change subject: IMPALA-4856: Port data stream service to KRPC

Patch Set 3:


Some more comments. Still going through the patch.
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
File be/src/runtime/

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 ?
File be/src/runtime/

PS3, Line 117: DCHECK(

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia66704be7a0a8162bb85556d07b583ec756c584b
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-HasComments: Yes

View raw message