Todd Lipcon has posted comments on this change. Change subject: KUDU-2065: Support cancellation for outbound RPC call ......................................................................
Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/7455/3/src/kudu/rpc/connection.h File src/kudu/rpc/connection.h: PS3, Line 280: MayBe nit: "Maybe" is one word rather than "MayBe" http://gerrit.cloudera.org:8080/#/c/7455/3/src/kudu/rpc/messenger.cc File src/kudu/rpc/messenger.cc: Line 448: if (controller.call_) { under what circumstance would controller not have a 'call_' member set? Wouldn't it be a bug on the user's part if they run Cancel on a controller which is current in the 'reset' state? I think it would be better to CHECK than to do a no-op, so that bug is caught early http://gerrit.cloudera.org:8080/#/c/7455/3/src/kudu/rpc/messenger.h File src/kudu/rpc/messenger.h: PS3, Line 199: const RpcController& I think taking this by pointer makes more sense since it "mutates" the state of the argument http://gerrit.cloudera.org:8080/#/c/7455/3/src/kudu/rpc/outbound_call.h File src/kudu/rpc/outbound_call.h: PS3, Line 264: triggeres typo PS3, Line 266: reference typo: references http://gerrit.cloudera.org:8080/#/c/7455/2/src/kudu/rpc/proxy.cc File src/kudu/rpc/proxy.cc: Line 106: << "It is illegal to call set_user_credentials() after request processing has started"; > Yes, the issue is that the RpcController doesn't really have a handle to th yea, it's a little messy vs having it directly in RpcController. Did you consider the other suggestion above of adding a messenger_ member to RpcController? It could be set in Proxy::AsyncRequest() at the same spot where it sets call_ and SetRequestParam() I think the lifetime is safe. -- 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: 3 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
