Todd Lipcon has posted comments on this change. Change subject: Allow to release an rpc transfer's data ......................................................................
Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/6592/5//COMMIT_MSG Commit Message: PS5, Line 10: obtain data for decoding what do you mean by that? http://gerrit.cloudera.org:8080/#/c/6592/5/src/kudu/rpc/outbound_call.h File src/kudu/rpc/outbound_call.h: PS5, Line 324: should should be obtained? or MUST be obtained? (i.e after releasing transfer data, is it legal to call other methods on this instance?) PS5, Line 323: // Returns a pointer to the transfer data, including the header and all : // sidecars. Pointers to individual sidecars should be obtained before : // calling this method. That is, this method is not meant to return : // data for decoding but rather to provide a way to release ownership : // of the whole transfer data. maybe move this doc to RpcController so it's user-facing, and just leave a breadcrumb here (like above) PS5, Line 326: // data for decoding but rather to provide a way to release ownership : // of the whole transfer data. : if it only is used for ownership, then why do we need to get a Slice instead of just the pointer to the start? i.e we're only expecting the user to later 'delete' this, right? http://gerrit.cloudera.org:8080/#/c/6592/5/src/kudu/rpc/rpc_controller.h File src/kudu/rpc/rpc_controller.h: PS5, Line 192: // Resets this controller, but releases the transfer data before. : Status ReleaseTransferDataAndReset(Slice* release_data); "transfer data" here is too implementation-centric. I think the name should probably be like ReleaseResponseBufferAndReset or somesuch. http://gerrit.cloudera.org:8080/#/c/6592/5/src/kudu/rpc/transfer.h File src/kudu/rpc/transfer.h: PS5, Line 87: // NOTE: buf_.release() doesn't memcpy if the transfer has completed, because : // buf_ must have been mandatorily resized from the initial capacity of 4 bytes : // to receive the whole message. : not sure I follow. The initial capacity of faststring is 32 bytes, so it's still possible it as to alloc and memcpy, but we don't really care, because the case of a <32b response is rare? -- To view, visit http://gerrit.cloudera.org:8080/6592 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
