Michael Ho has posted comments on this change. Change subject: KUDU-2065: Support cancellation for outbound RPC call ......................................................................
Patch Set 2: (17 comments) http://gerrit.cloudera.org:8080/#/c/7455/2//COMMIT_MSG Commit Message: PS2, Line 9: Proxy::Cancel() > seems a bit odd to have it in the Proxy rather than just on the controller Removed this interface and now use the messenger's interface directly. http://gerrit.cloudera.org:8080/#/c/7455/2/java/kudu-client/src/main/java/org/apache/kudu/client/Status.java File java/kudu-client/src/main/java/org/apache/kudu/client/Status.java: Line 242: public static Status Cancelled(String msg) { > I think we should use Aborted here rather than adding a new one. Adding a n Done http://gerrit.cloudera.org:8080/#/c/7455/2/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: PS2, Line 265: FindOrNull > you can use FindPtrOrNull here to avoid the double-pointer Done PS2, Line 296: } else if (call_->cancellation_requested()) { : call_->SetCancelled(); : > in this case I think it would make sense to call SetSent() and _then_ call Done. Also moved the check into SetSent(). Line 627: if (car->call.get() == nullptr) { > see tidy warning -- we prefer to just use the implicit bool cast on smart p Done PS2, Line 631: "already timed out") > can you update this message to be more accurate? I don't think it would eve Done http://gerrit.cloudera.org:8080/#/c/7455/2/src/kudu/rpc/connection.h File src/kudu/rpc/connection.h: PS2, Line 280: Try > I think 'Maybe' is better than 'Try' (try makes me think of something like Done http://gerrit.cloudera.org:8080/#/c/7455/2/src/kudu/rpc/outbound_call.cc File src/kudu/rpc/outbound_call.cc: PS2, Line 52: debug build > I don't think you need to be specific to debug builds, it's good to test fa Done Line 370: std::lock_guard<simple_spinlock> l(lock_); > worth a DCHECK(!IsFinished()) here I think Done http://gerrit.cloudera.org:8080/#/c/7455/2/src/kudu/rpc/outbound_call.h File src/kudu/rpc/outbound_call.h: PS2, Line 160: waiting > "is waiting" Done PS2, Line 160: finished > insert "has" Done Line 163: void Cancel(); > can you add something like: "REQUIRES: must be called from the reactor thre Done PS2, Line 185: client > the client Done Line 233: return cancellation_requested_; > is this always accessed by the reactor thread? if not, does it need some sy Yes but there is no easy way to DCHECK it here. Comments added. http://gerrit.cloudera.org:8080/#/c/7455/2/src/kudu/rpc/proxy.cc File src/kudu/rpc/proxy.cc: Line 106: messenger_->QueueCancellation(controller->call_); > I commented elsewhere that I don't think this is great to have in the Proxy Yes, the issue is that the RpcController doesn't really have a handle to the messenger. I deleted this interface and added an interface Messenger::Cancel(const RpcController&). I had the impression from the comment in Messenger class that it's breaking the abstraction to pass RpcController to Messenger directly but actually sounds okay based on your comment. http://gerrit.cloudera.org:8080/#/c/7455/2/src/kudu/rpc/reactor.h File src/kudu/rpc/reactor.h: PS2, Line 242: is > as Done http://gerrit.cloudera.org:8080/#/c/7455/2/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: Line 890: TEST_P(TestRpc, TestCancellation) { > this is a nice test, but would be good to also add more of a "stress test" Good idea. Working on it now. -- To view, visit http://gerrit.cloudera.org:8080/7455 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaf53c5b113de10d573bd32fb9b2293572e806fbf Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
