Alexey Serbin 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 2: Code-Review+1 (5 comments) 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 CallbackBehavior nit: consider using 'enum class' just to be more type-safe 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@98 PS2, Line 98: DVLOG(4) << "OutboundCall " << this << " constructed with state_: " << StateName(state_) : << " and RPC timeout: " : << (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 sense to introduce an utility method to do this and the other constructor's work with some variation? http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/outbound_call.cc@128 PS2, Line 128: this nit: might ToString() fit better here, or the whole idea was to track just pointers? http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/outbound_call.cc@128 PS2, Line 128: DVLOG(4) << "OutboundCall " << this << " constructed with state_: " << StateName(state_) : << " and RPC timeout: " : << (controller->timeout().Initialized() ? controller->timeout().ToString() : "none"); nit: use Substitute() for format the message for better readability in the code? 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? -- 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: 2 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: Fri, 01 Oct 2021 19:31:48 +0000 Gerrit-HasComments: Yes
