impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] KUDU-2065: Support cancellation for outbound RPC call
Date Tue, 29 Aug 2017 16:57:04 GMT
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 <kwho@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