Henry Robinson has posted comments on this change. Change subject: KUDU-1866: 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 earli Done 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). Th Done 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 Done PS4, Line 199: push_back > we usually use emplace_back with std::move, though it probably turns into t Done 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? Done Line 237: Slice sidecar_slices_[TransferLimits::kMaxPayloadSlices]; > and rename this to request_sidecars? Done 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 wou Done 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 o Yep, combined into SetRequestParam(msg, sidecars); 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 her Combined with SetRequestParam() 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 Done 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 t instead, I have controller set the call params directly, so we don't need to worry about ownership transfer here. 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'); Done Line 468: controller.AddOutboundSidecar(RpcSidecar::FromSlice(Slice(s1)), &idx1); > should these be CHECK_OK? Done 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::kMaxSid Done Line 120: return Status::ServiceUnavailable("All available sidecars already used"); > how about RuntimeError? this is more akin to an OOM than a "service unavail Done 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 Done 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 throu Done Line 48: static gscoped_ptr<RpcSidecar> FromFaststring(gscoped_ptr<faststring> data); > can we use unique_ptr here since it's new code? Done Line 55: Slice buffer, Slice* sidecars); > hrm, is this a C-style array output? we should clarify how long it should b Changing that to a vector led to a bit of yak-shaving. There's a comment on OutboundTransfer::payload_slices_ that says using an array saved a couple % in performance. It's not totally clear to me that changing the *Call objects to use vector wouldn't therefore degrade perf in the same way (isn't there usually 1 : 1 mapping between calls and transfers?). So anyhow, I left a comment, and documented the limitation. http://gerrit.cloudera.org:8080/#/c/5908/4/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: Line 123: using kudu::server::HybridClock; > warning: using decl 'HybridClock' is unused [misc-unused-using-decls] 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: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
