Todd Lipcon has posted comments on this change. Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout ......................................................................
Patch Set 2: (22 comments) http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: Line 251: car->transfer->Abort(status); what about in the case of talking to an old server which doesn't support footer, but still mid-transmission? in that case don't we want to no-op this function call? here it seems like you are still aborting it, which would result in calling the callback while the sidecar is still referenced, and thus hitting the original bug again. PS2, Line 268: // Cancel or abort any outbound transfer for this call so references to the sidecars : // can be relinquished. per above, this is still best-effort in the case that you are talking to a remote which doesn't support this feature. PS2, Line 703: if (!transfer->TransferStarted()) { : if (transfer->is_for_outbound_call()) { : if (!StartCallTransfer(transfer)) { these can all be collapsed into a single boolean expression now, right? http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.h File src/kudu/rpc/connection.h: PS2, Line 199: takes typo: take PS2, Line 199: Callers should takes this into account. not quite sure what this sentence is indicating. Don't we assume that callers should always take everything documented into account? :) PS2, Line 201: negotiation_complete_ && is negotiation_complete_ necessary? before negotiation is complete, isn't remote_features_ guaranteed to be an empty set, in which case it will already be false? PS2, Line 284: sending it for : // the first time I think better to say "beginning to send it" or "beginning transmission" or somesuch Line 288: // If the checks pass, the outbound call will be set to 'SENDING' state. if the checks don't pass, is this responsible for 'finishing' the call? eg if it doesn't support the feature flags, will this take care of calling the callback? PS2, Line 289: Append nit: "Appends" PS2, Line 303: if the transfer is for an outbound call. not quite clear here which part of the sentence the "if ..." applies to. If I understand correctly, maybe this should say: // Remove the front transfer from the outbound transfers queue. // If the transfer is associated with an outbound call, clears the transfer reference from the associated CallAwaitingResponse. or something like that, to be more clear that it _always_ removes the front, and then the reference removal is in addition to that, in certain circumstances. http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/outbound_call.cc File src/kudu/rpc/outbound_call.cc: Line 30: #include "kudu/rpc/connection.h" fix sort order PS2, Line 260: shutted nit: "shut" Line 266: // fall-through if remote supports parsing footer. we have a special macro here 'FALLTHROUGH_INTENDED' which turns into something magic in clang (see gutil/macros.h) PS2, Line 352: delete [] header_buf_.release(); : delete [] footer_buf_.release(); looking at the implementation of faststring::release() I think these calls are actually not a great idea, since, if the data is less than 32 bytes, it will be stored inline in the faststring object, and then these 'release' calls actually cause an allocation and copy into a heap buffer. For the header, I think it's typically >32 bytes so this is beneficial, but for the footer it's almost certainly not. I think it's better to change both of these to call clear() followed by shrink_to_fit(), which will free the memory only if it's a separate allocation. http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/outbound_call.h File src/kudu/rpc/outbound_call.h: PS2, Line 159: send_footer is this out of date? I don't see this param Line 171: // to indicate that the call has been aborted. worth indicating whether this call requires that AppendFooter() has previously been called. What happens if no AppendFooter() was called? would it CHECK fail? (probably should?) PS2, Line 210: IsBeingSent hrm, I actually am going to disagree with my previous comment and still think this isn't a great name, since it includes the "scheduled but hasn't started sending". Maybe "IsQueuedForTransfer" or 'IsOnTransferQueue' or something? http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/rpc_header.proto File src/kudu/rpc/rpc_header.proto: PS2, Line 274: Note that the footer is designed to be modified : // after the initial serialization and it will be re-serialized after modification. : // To avoid unexpected change in the total message size, keep to using fixed sized : // fields only in the footer. move this comment to be below the 'aborted' field, since it documents any new fields that might be added, and isn't documenting 'aborted' itself http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/serialization.h File src/kudu/rpc/serialization.h: Line 78: void IncrementMsgLength(size_t inc_len, > warning: function 'kudu::rpc::serialization::IncrementMsgLength' has a defi please fix this nit http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/transfer.cc File src/kudu/rpc/transfer.cc: Line 78: /* Initialize the dummy buffer with a known pattern for easier debugging. */ style: // C++ style comment Line 190: ++n_payload_slices_; how about a post-increment on the prior line? Line 223: // the transfer if only the footer is left. could this last bit result in a "hung" cancellation in the case that the server read all of the bytes except for the footer and then hung? we can't cancel because the call hasn't been sent, but it also won't be fully sent for a while because of the hung peer. Even though we've already sent the sidecars and thus cancelling doesn't save us on that lifecycle issue, it does seem like we'd still want to more aggressively call the callback -- To view, visit http://gerrit.cloudera.org:8080/7599 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc 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
