Todd Lipcon has posted comments on this change. Change subject: KUDU-1866: Add request-side sidecars ......................................................................
Patch Set 8: (13 comments) http://gerrit.cloudera.org:8080/#/c/5908/8/src/kudu/rpc/rpc_controller.cc File src/kudu/rpc/rpc_controller.cc: PS8, Line 125: push_back nit: emplace_back http://gerrit.cloudera.org:8080/#/c/5908/8/src/kudu/rpc/rpc_controller.h File src/kudu/rpc/rpc_controller.h: PS8, Line 215: // Set the outbound call_'s request parameter, and transfer ownership of : // outbound_sidecars_ to call_ in preparation for serialization. : void SetRequestParam(const google::protobuf::Message& req); nit: we usually put all the methods above all the fields PS8, Line 219: // Move all sidecars into call_ once it is set. : // void TransferOutboundSidecars(); remove this http://gerrit.cloudera.org:8080/#/c/5908/8/src/kudu/rpc/rpc_header.proto File src/kudu/rpc/rpc_header.proto: Line 214: repeated uint32 sidecar_offsets = 16; I thought you needed uint64 for some reason? http://gerrit.cloudera.org:8080/#/c/5908/8/src/kudu/rpc/rpc_sidecar.cc File src/kudu/rpc/rpc_sidecar.cc: Line 35: virtual Slice AsSlice() const { return slice_; } use 'override' Line 44: virtual Slice AsSlice() const { return *data_; } same Line 69: if (last >= 0) { I think instead of this condition, it would be cleaner to stick a: if (offsets.size() == 0) return Status::OK(); ... at the top of the function. We try to avoid indented blocks where it's easy to by using 'early outs' instead PS8, Line 83: (next_offset - cur_offset)) this expression (and the one above) has some weird overflow behavior, since it's unsigned. perhaps we should just use int64_t above for the offsets to make it less finicky (I see you just moved this code, sorry for nit-picking, but may as well fix it while we're looking at it) PS8, Line 95: sidecars[last] = : Slice(buffer.data() + cur_offset, buffer.size() - cur_offset); : nit: no need to wrap, is there? http://gerrit.cloudera.org:8080/#/c/5908/8/src/kudu/rpc/rpc_sidecar.h File src/kudu/rpc/rpc_sidecar.h: PS8, Line 51: + "and" http://gerrit.cloudera.org:8080/#/c/5908/8/src/kudu/rpc/rtest.proto File src/kudu/rpc/rtest.proto: Line 68: message PushTwoStringsRequestPB { nit: maybe SendTwoSidecarsPB or something? Not sure what "push" vs "send" means http://gerrit.cloudera.org:8080/#/c/5908/8/src/kudu/rpc/transfer.h File src/kudu/rpc/transfer.h: Line 53: kMaxPayloadSlices = 12 // == kMaxSidecars + 2 (header + msg) I think you should be able to just write kMaxSidecars + 2 Line 55: }; perhaps DISABLE_IMPLICIT_CONSTRUCTORS(TransferLimits) here since it' not meant to be instantiated -- To view, visit http://gerrit.cloudera.org:8080/5908 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3d709edb2a22dc983f51b69d7660a39e8d8d6a09 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
