Sailesh Mukil has posted comments on this change. Change subject: KUDU-2065: Support cancellation for outbound RPC call ......................................................................
Patch Set 1: (3 comments) Sorry for the delay, I wanted to take some time to understand this well. I just have a few clarifying questions to just ensure that we've thought through this thoroughly. I'm not proposing any code changes at this point. 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 relying on the fact that the Cancel() and Send() will be processed by the same reactor thread and therefore, they can't race? 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 for debuggability reasons, so we know the difference between an RPC level timeout vs an application level timeout (if we choose to add one) 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 if the logic checks out: If we asynchronously call Cancel() from the DataStreamSender, before this function has actually started executing, we would be hitting DCHECKs in RpcController::Cancel(). Is there a way we are going to make sure that doesn't happen, since the RPC layer doesn't synchronize between this and Cancel() itself? -- 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: Kudu Jenkins Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
