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] KUDU-2065: Support cancellation for outbound RPC call
Date Tue, 29 Aug 2017 17:52:04 GMT
Michael Ho has posted comments on this change.

Change subject: KUDU-2065: Support cancellation for outbound RPC call
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7743/1/be/src/kudu/rpc/connection.cc
File be/src/kudu/rpc/connection.cc:

PS1, Line 269: car->call.reset();
> How do we ensure that we don't do this while the call is sending? Are we re
Yes, it's an implicit assumption in Kudu RPC to map the same connection to the same reactor
thread. Please see Messenger::RemoteToReactor(const Sockaddr &remote);


PS1, Line 626: has already timed out or has already been cancelled
> I think at some point, it would be good to differentiate between the two fo
If you look at the OutboundCall::SetTimedOut() and OutboundCall::SetCancelled(), we do include
different status details for the two different cases.


http://gerrit.cloudera.org:8080/#/c/7743/1/be/src/kudu/rpc/proxy.cc
File be/src/kudu/rpc/proxy.cc:

Line 85:   controller->SetMessenger(messenger_.get());
> The following would be a bug for other reasons, but I just want to confirm 
This function is expected to be invoked from the same thread which calls cancellation. My
understanding is that DSS (or generally speaking the execution thread of a fragment instance)
is single threaded. This will be invoke from KrpcDataStreamSender::Close() to cancel any in-flight
RPC. 

We rely on rpc_in_flight_ as a synchronization and it's protected by a lock. Cancellation
needs to hold the lock, check rpc_in_flight_ is true before requesting cancellation.

DSS is mostly single threaded so the only race is with the reactor thread call-back. The window
in which this problem can happen is when we need to reset the RPC controller to retry the
RPC (arguably, the reset may not be necessary) but that will happen under a lock anyway.

There are actually DCHECKs to make sure call_ and messenger_ aren't nullptr in RpcController::Cancel().


-- 
To view, visit http://gerrit.cloudera.org:8080/7743
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf53c5b113de10d573bd32fb9b2293572e806fbf
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <todd@apache.org>
Gerrit-HasComments: Yes

Mime
View raw message