impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
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:

(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 <henry@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message