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 <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
