Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17892 )
Change subject: [rpc] allow reuse of outbound request buffers when retrying ...................................................................... Patch Set 3: (22 comments) http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/mt-rpc-test.cc File src/kudu/rpc/mt-rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/mt-rpc-test.cc@67 PS2, Line 67: void SingleCall(Sockaddr server_addr, const string& method_name, > warning: the parameter 'server_addr' is copied for each invocation but only Ack http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/mt-rpc-test.cc@79 PS2, Line 79: void HammerServer(Sockaddr server_addr, const string& method_name, > warning: the parameter 'server_addr' is copied for each invocation but only Ack http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/mt-rpc-test.cc@86 PS2, Line 86: void HammerServerWithMessenger( > warning: method 'HammerServerWithMessenger' can be made static [readability Ack http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/mt-rpc-test.cc@87 PS2, Line 87: Sockaddr server_addr, const string& method_name, Status* last_result, > warning: the parameter 'server_addr' is copied for each invocation but only Ack http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/outbound_call.h File src/kudu/rpc/outbound_call.h: http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/outbound_call.h@124 PS2, Line 124: enum class CallbackBe > nit: consider using 'enum class' just to be more type-safe Done http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/outbound_call.cc File src/kudu/rpc/outbound_call.cc: http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/outbound_call.cc@83 PS2, Line 83: const RemoteMethod& remote_method, > warning: pass by value and use std::move [modernize-pass-by-value] Ack http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/outbound_call.cc@98 PS2, Line 98: DVLOG(4) << Substitute("OutboundCall $0 constructed with the state_: $1 and RPC timeout: $2", : this, StateName(state_), : (controller->timeout().Initialized() ? controller->timeout().ToString() : "none")); : payload_->header_.set_call_id(kInvalidCallId); : start_time_ = MonoTime::Now(); : : if (!controller_->required_server_features().empty()) { : required_rpc_features_.insert(RpcFeatureFlag::APPLICATION_FEATURE_FLAGS); : } : : if (controller_->request_id_) { : payload_->header_.set_allocated_request_id(controller_->request_id_.release()); : } > This looks similar to what's used in the other constructor. Does it make s Done http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/outbound_call.cc@128 PS2, Line 128: > nit: might ToString() fit better here, or the whole idea was to track just I do think this is meant to track the pointer value. It does come in handy when debugging memory leaks and races. http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/outbound_call.cc@128 PS2, Line 128: : void OutboundCall::SerializeTo(TransferPayload* slices) { : DCHECK_LT(0, payload_->request_buf_.size()) > nit: use Substitute() for format the message for better readability in the Done http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/outbound_call.cc@509 PS2, Line 509: resp->set_state(RpcCallInProgressPB::SENT); > warning: parameter 'req' is unused [misc-unused-parameters] Ack http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/proxy.cc File src/kudu/rpc/proxy.cc: http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/proxy.cc@229 PS2, Line 229: (PREDICT_FALSE(!con > IIUC, RpcController::Reset() sets outbound_sidecars_total_bytes_ to 0, but >From what I can tell this is OK -- outbound_sidecars_total_bytes_ is useful >when adding more outbound sidecars, to ensure we don't go over the max >transfer bytes limit. In this case, we aren't adding more sidecars -- just >reusing the same payload. http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/rpc-test-base.h File src/kudu/rpc/rpc-test-base.h: http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/rpc-test-base.h@94 PS2, Line 94: static const std::string kAddMethodName; : static const std::string kSleepMethodName; : static const std::string kSleepWithSidecarMethodName; : static const std::string kPushStringsMethodName; : static const std::string kSendTwoStringsMethodName; : static const std::string kAddExactlyOnce; > Just curious why it was necessary to turn those into std::string? Updated these in order to capture method names as references in lambdas. Otherwise we'd see issues when converting const char* to string on the stack, and the stack getting blown away when the lambda is executed. http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/rpc-test-base.h@403 PS2, Line 403: const std::string GenericCalculatorService::kFullServiceName = > warning: variable 'kFullServiceName' defined in a header file; variable def Ack http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/rpc-test-base.h@405 PS2, Line 405: const std::string GenericCalculatorService::kAddMethodName = "Add"; > warning: variable 'kAddMethodName' defined in a header file; variable defin Ack http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/rpc-test-base.h@406 PS2, Line 406: const std::string GenericCalculatorService::kSleepMethodName = "Sleep"; > warning: variable 'kSleepMethodName' defined in a header file; variable def Ack http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/rpc-test-base.h@407 PS2, Line 407: const std::string GenericCalculatorService::kSleepWithSidecarMethodName = "SleepWithSidecar"; > warning: variable 'kSleepWithSidecarMethodName' defined in a header file; v Ack http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/rpc-test-base.h@408 PS2, Line 408: const std::string GenericCalculatorService::kPushStringsMethodName = "PushStrings"; > warning: variable 'kPushStringsMethodName' defined in a header file; variab Ack http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/rpc-test-base.h@409 PS2, Line 409: const std::string GenericCalculatorService::kSendTwoStringsMethodName = "SendTwoStrings"; > warning: variable 'kSendTwoStringsMethodName' defined in a header file; var Ack http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/rpc-test-base.h@410 PS2, Line 410: const std::string GenericCalculatorService::kAddExactlyOnce = "AddExactlyOnce"; > warning: variable 'kAddExactlyOnce' defined in a header file; variable defi Ack http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/rpc-test-base.h@412 PS2, Line 412: const char *GenericCalculatorService::kFirstString = > warning: variable 'kFirstString' defined in a header file; variable definit Ack http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/rpc_controller.cc File src/kudu/rpc/rpc_controller.cc: http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/rpc_controller.cc@172 PS2, Line 172: outbound_sidecars_tota > What about zeroing outbound_sidecars_total_bytes_: should it be done here a Done http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/rpc_controller.cc@176 PS2, Line 176: std::vector<unique_ptr<RpcSidecar>> Rpc > ditto Done -- To view, visit http://gerrit.cloudera.org:8080/17892 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I118f3559c4647bdd996617443bd371a041711295 Gerrit-Change-Number: 17892 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 06 Oct 2021 00:17:53 +0000 Gerrit-HasComments: Yes
