Todd Lipcon has posted comments on this change. Change subject: KUDU-2065: Support cancellation for outbound RPC call ......................................................................
Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/7455/4/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: Line 966: uint32_t payload[256]; can you introduce a constant for this payload size since the magic number 256 shows up a few places? I think a much larger payload is also more likely to trigger bugs (eg 8M or something) since it's more likely that the socket writes will be split into multiple calls Line 982: SleepFor(MonoDelta::FromMicroseconds(i * 30)); > warning: either cast from 'int' to 'int64_t' (aka 'long') is ineffective, o maybe use a Random instance here? (util/random.h) to randomize the sleep between 0 and i*30 just to cover more possibilities? http://gerrit.cloudera.org:8080/#/c/7455/4/src/kudu/rpc/rpc_controller.h File src/kudu/rpc/rpc_controller.h: PS4, Line 227: Should only be called when : // there is an outstanding outbound call. I think this doc should be clarified a bit to include the following important bits: - it's always safe to call Cancel() after you've sent a call, so long as you haven't called Reset() yet. (ie you aren't responsible for synchronizing this against the callback) - it is "best effort" - ie it's still possible that your callback will be fired with a success Status - if cancellation is successful your callback will be called with a Aborted status - it is async (the callback will still be called by another thread) -- 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: 4 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
