Todd Lipcon has posted comments on this change. Change subject: KUDU-1866: [WIP] Add request-side sidecars ......................................................................
Patch Set 4: (20 comments) http://gerrit.cloudera.org:8080/#/c/5908/4//COMMIT_MSG Commit Message: Line 7: KUDU-1866: [WIP] Add request-side sidecars can you remove 'WIP' from the commit message? when we chatted offline earlier I think you said this is ready to go from your side. Line 9: This patch adds sidecars to client requests. Using the same mechanism as on the nit: can you wrap the commit message at a narrower width? (eg 72 chars). This is overflowing the gerrit screen a bit. http://gerrit.cloudera.org:8080/#/c/5908/4/src/kudu/rpc/inbound_call.cc File src/kudu/rpc/inbound_call.cc: PS4, Line 69: int last = header_.sidecar_offsets_size() - 1; : if (last >= TransferLimits::kMaxPayloadSlices) { this strikes me as a bit funny instead of just "if (header_.sidecar_offsets_size() > TransferLimits::kMaxPayloadSlices) Also, isn't kMayPayloadSlices actually inclusive of the request itself, and the 'max sidecars' is a lower value? See the comment around line 193 PS4, Line 199: push_back we usually use emplace_back with std::move, though it probably turns into the same code http://gerrit.cloudera.org:8080/#/c/5908/4/src/kudu/rpc/inbound_call.h File src/kudu/rpc/inbound_call.h: Line 233: std::vector<gscoped_ptr<RpcSidecar>> sidecars_; can you rename this to response_sidecars or something so it's clearer? Line 237: Slice sidecar_slices_[TransferLimits::kMaxPayloadSlices]; and rename this to request_sidecars? (or inbound/outbound sidecars) http://gerrit.cloudera.org:8080/#/c/5908/4/src/kudu/rpc/outbound_call.cc File src/kudu/rpc/outbound_call.cc: PS4, Line 101: size_t param_len = request_buf_.size(); : if (PREDICT_FALSE(param_len == 0)) { : return Status::InvalidArgument("Must call SetRequestParam() before SerializeTo()"); : } I think this was placed at the top so that, if it returned an error, it would have no side effect. Now it would have the side effect of half-initializing header_. On the other hand, now that I look at this code again, it could probably just be a CHECK or DCHECK, since it would be an internal programmer error if we hit this, right? PS4, Line 116: uint32_t message_size = req.ByteSize(); : for (const gscoped_ptr<RpcSidecar>& car: sidecars_) { : header_.add_sidecar_offsets(sidecar_byte_size_ + message_size); : sidecar_byte_size_ += car->AsSlice().size(); : } : : it strikes me as a little "spaghetti" to do this here - we're now relying on a very specific ordering that the sidecars have to get set before the RequestParam, right? Maybe we should combine SetRequestParam and the new SetSidecars function? is that doable? http://gerrit.cloudera.org:8080/#/c/5908/4/src/kudu/rpc/outbound_call.h File src/kudu/rpc/outbound_call.h: PS4, Line 207: TransferOutboundSidecars I think 'SetOutboundSidecars' or just 'set_outbound_sidecars' is better here. 'Transfer' makes it sound like it's actually writing something to the socket, etc whereas this is just a setter (never mind that it's a setter that "moves"). PS4, Line 310: // Total size in bytes of all sidecars in 'sidecars_'. Set in SetRequestParam(). : uint32_t sidecar_byte_size_ = 0; I think it would be safer to use a signed int64_t here and initialize it to -1. Then you can add DCHECKs in other places that it's not -1, etc. http://gerrit.cloudera.org:8080/#/c/5908/4/src/kudu/rpc/proxy.cc File src/kudu/rpc/proxy.cc: PS4, Line 84: controller->TransferOutboundSidecars(); : call->SetRequestParam(req); per suggestion elsewhere, I think this could work well if we combined the two, eg something like: call->SetRequestParam(req, controller->TakeOutboundSidecars()); (where TakeOutboundSidecars returns an rvalue reference) http://gerrit.cloudera.org:8080/#/c/5908/4/src/kudu/rpc/rpc-test-base.h File src/kudu/rpc/rpc-test-base.h: Line 467: string s1; s1.resize(size1); I think there's a constructor like: string s1(size1, 'x'); Line 468: controller.AddOutboundSidecar(RpcSidecar::FromSlice(Slice(s1)), &idx1); should these be CHECK_OK? PS4, Line 480: CHECK_EQ(size1, resp.size1()); : CHECK_EQ(size2, resp.size2()); : instead of just checking size equality, maybe have the test service just "echo" the strings and you can check actual data equality too? that would help catch if you had an offset calculation error http://gerrit.cloudera.org:8080/#/c/5908/4/src/kudu/rpc/rpc_controller.cc File src/kudu/rpc/rpc_controller.cc: Line 119: if (outbound_sidecars_.size() + 2 > TransferLimits::kMaxPayloadSlices) { we have this magic in a few places, let's add a new TransferLimits::kMaxSidecars or something Line 120: return Status::ServiceUnavailable("All available sidecars already used"); how about RuntimeError? this is more akin to an OOM than a "service unavailable" (which implies it might become available later) http://gerrit.cloudera.org:8080/#/c/5908/4/src/kudu/rpc/rpc_sidecar.h File src/kudu/rpc/rpc_sidecar.h: Line 24: #include <google/protobuf/repeated_field.h> nit: should be included before any of our own headers PS4, Line 44: // After reconstructing the array of sidecars, servers may retrieve the sidecar data : // through the RpcContext or RpcController interfaces. : c should probably say 'servers or clients may retrieve the sidecar data through the RpcContext or RpcController respectively' Line 48: static gscoped_ptr<RpcSidecar> FromFaststring(gscoped_ptr<faststring> data); can we use unique_ptr here since it's new code? Line 55: Slice buffer, Slice* sidecars); hrm, is this a C-style array output? we should clarify how long it should be in the docs (or maybe just switch to a vector) -- 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: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
