Henry Robinson has posted comments on this change. Change subject: KUDU-1866: Add request-side sidecars ......................................................................
Patch Set 8: (13 comments) Thanks for the review! 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 Done 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 Done PS8, Line 219: // Move all sidecars into call_ once it is set. : // void TransferOutboundSidecars(); > remove this Done 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? Done (seems little reason not to). Hard to add a test because rpc_max_message_size is 32-bit. 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' Done Line 44: virtual Slice AsSlice() const { return *data_; } > same Done Line 69: if (last >= 0) { > I think instead of this condition, it would be cleaner to stick a: Done PS8, Line 83: (next_offset - cur_offset)) > this expression (and the one above) has some weird overflow behavior, since Still tricky because of the change to uint64_t for offsets - I changed the message to avoid the subtraction, and I think it's clearer for that. BTW - will Kudu always be compiled on 64-bit machines (i.e. where sizeof(size_t) >= 8)? Otherwise changing to 64-bit offsets is more challenging because of the implicit limitations on the size of a slice. PS8, Line 95: sidecars[last] = : Slice(buffer.data() + cur_offset, buffer.size() - cur_offset); : > nit: no need to wrap, is there? Done 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" Done 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" m 'Send' as used above seems to mean 'please respond to my request with two strings'. Here I want a verb that means 'please accept these two strings'. I added a comment; maybe that's clear enough? 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 Done Line 55: }; > perhaps DISABLE_IMPLICIT_CONSTRUCTORS(TransferLimits) here since it' not me Done -- 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
