Todd Lipcon has posted comments on this change. Change subject: KUDU-2065: Support cancellation for outbound RPC call ......................................................................
Patch Set 2: (19 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 itself like rpc->Cancel() Usually we think of an RPC as a <req, resp, controller> tuple, and the proxy is a short-lived thing which doesn't necessarily live as long as the controller. The controller is meant to be the user-facing handle that uniquely identifies a call in progress. 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 new one has implications for API, wire, and ABI compatibility (this is part of our public API with compat guarantees) 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 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 SetCancelled(), because the application may want to know "was it possible that the remote end may have received this call" and thus determine whether it can safely retry a non-idempotent request. Currently we don't track any boolean like 'was_sent_' but if we start doing so, doing it the above-mentioned way will ensure that it is accurate. Line 627: if (car->call.get() == nullptr) { > warning: redundant get() call on smart pointer [google-readability-redundan see tidy warning -- we prefer to just use the implicit bool cast on smart pointer null checks PS2, Line 631: "already timed out") can you update this message to be more accurate? I don't think it would ever bubble up, but in case it does I think it would be nicer to say "or cancelled", or say "RPC call no longer needed to be sent" or "obsolete" or something 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 a TryLock where you expect it to usually succeed, and return a boolean indicating whether it succeeded) 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 failure cases in both builds and I don't think the perf is a concern PS2, Line 215: || state_ == SENDING if you take my suggestion elsewhere about going SENDING -> SENT -> CANCELLED rather than directly SENDING -> CANCELLED then you can make this assertion tigher PS2, Line 229: bool OutboundCall::ShouldInjectCancellation() const { : return FLAGS_rpc_inject_cancellation_state != -1 && : FLAGS_rpc_inject_cancellation_state == state(); : } I think it's worth moving this to the header to get the inline call Line 370: std::lock_guard<simple_spinlock> l(lock_); worth a DCHECK(!IsFinished()) here I think 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" PS2, Line 160: finished insert "has" Line 163: void Cancel(); can you add something like: "REQUIRES: must be called from the reactor thread" PS2, Line 185: client the client Line 233: return cancellation_requested_; is this always accessed by the reactor thread? if not, does it need some synchronization or be made into an AtomicBool? 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, vs in the RpcController directly. I guess the issue is that the RpcController doesn't remember a reference t its messenger but the proxy does? I think even making this a call directly on Messenger instead of Proxy is a bit cleaner since there isn't actually any restriction that the Proxy object used to call cancel has any relation to the proxy object used to send a call. Alternatively, what about adding a raw-pointer to Messenger in the RpcController after a call is initiated? We already have a guarantee that the Messenger outlives any calls on it, so the raw pointer is safe in terms of lifetime, and it allows for a cleaner interface RpcController::Cancel() http://gerrit.cloudera.org:8080/#/c/7455/2/src/kudu/rpc/reactor.h File src/kudu/rpc/reactor.h: PS2, Line 242: is as 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" variant which uses the actual user-facing APIs from a separate thread to inject cancellation, like we'd do in real life. Check out mt-rpc-test.cc for some multithreaded stress-style tests. -- 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 <k...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
