You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2021/10/01 01:32:19 UTC
[kudu-CR] [rpc] allow reuse of outbound request buffers when retrying
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17892
Change subject: [rpc] allow reuse of outbound request buffers when retrying
......................................................................
[rpc] allow reuse of outbound request buffers when retrying
This patch fixes a heap-use-after-free bug in the address-re-resolving
proxy caused by the fact that requests may be stack allocated, are
serialized once when making the OutboundCall, and then thrown away once
calling the (typically) user-provided callback.
This patch splits out the state that may be used by retries (i.e. the
serialized request, header, and sidecars) into its own class, and passes
this around when retrying.
There are some methods that don't appear to be used in this codebase,
but I opted to keep existing behavior because they seem to be used in
other codebases that share library code. Specifically, I don't see usage
of RpcController::AddOutboundSidecar(), but this does seem to be used in
Impala.
Change-Id: I118f3559c4647bdd996617443bd371a041711295
---
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/proxy-test.cc
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/proxy.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
9 files changed, 282 insertions(+), 79 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/17892/1
--
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: newchange
Gerrit-Change-Id: I118f3559c4647bdd996617443bd371a041711295
Gerrit-Change-Number: 17892
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] [rpc] allow reuse of outbound request buffers when retrying
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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: Verified+1
Failed test was from the known flaky test ConcurrentRebalancersTest.TwoConcurrentRebalancers/1
--
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 <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 06 Oct 2021 22:12:44 +0000
Gerrit-HasComments: No
[kudu-CR] [rpc] allow reuse of outbound request buffers when retrying
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17892
to look at the new patch set (#2).
Change subject: [rpc] allow reuse of outbound request buffers when retrying
......................................................................
[rpc] allow reuse of outbound request buffers when retrying
This patch fixes a heap-use-after-free bug in the address-re-resolving
proxy caused by the fact that requests may be stack allocated, are
serialized once when making the OutboundCall, and then thrown away once
calling the (typically) user-provided callback.
This patch splits out the state that may be used by retries (i.e. the
serialized request, header, and sidecars) into its own class, and passes
this around when retrying.
There are some methods that don't appear to be used in this codebase,
but I opted to keep existing behavior because they seem to be used in
other codebases that share library code. Specifically, I don't see usage
of RpcController::AddOutboundSidecar(), but this does seem to be used in
Impala.
Change-Id: I118f3559c4647bdd996617443bd371a041711295
---
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/proxy-test.cc
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/proxy.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
9 files changed, 282 insertions(+), 79 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/17892/2
--
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: newpatchset
Gerrit-Change-Id: I118f3559c4647bdd996617443bd371a041711295
Gerrit-Change-Number: 17892
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [rpc] allow reuse of outbound request buffers when retrying
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17892 )
Change subject: [rpc] allow reuse of outbound request buffers when retrying
......................................................................
[rpc] allow reuse of outbound request buffers when retrying
This patch fixes a heap-use-after-free bug in the address-re-resolving
proxy caused by the fact that requests may be stack allocated, are
serialized once when making the OutboundCall, and then thrown away once
calling the (typically) user-provided callback.
This patch splits out the state that may be used by retries (i.e. the
serialized request, header, and sidecars) into its own class, and passes
this around when retrying.
There are some methods that don't appear to be used in this codebase,
but I opted to keep existing behavior because they seem to be used in
other codebases that share library code. Specifically, I don't see usage
of RpcController::AddOutboundSidecar(), but this does seem to be used in
Impala[1].
1. https://github.com/apache/impala/blob/b28da054f3595bb92873433211438306fc22fbc7/be/src/rpc/sidecar-util.h#L48
Change-Id: I118f3559c4647bdd996617443bd371a041711295
Reviewed-on: http://gerrit.cloudera.org:8080/17892
Tested-by: Andrew Wong <aw...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/proxy-test.cc
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/proxy.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
9 files changed, 272 insertions(+), 82 deletions(-)
Approvals:
Andrew Wong: Verified
Alexey Serbin: Looks good to me, approved
--
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: merged
Gerrit-Change-Id: I118f3559c4647bdd996617443bd371a041711295
Gerrit-Change-Number: 17892
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [rpc] allow reuse of outbound request buffers when retrying
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 01 Oct 2021 19:31:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] [rpc] allow reuse of outbound request buffers when retrying
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: [rpc] allow reuse of outbound request buffers when retrying
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
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: deleteVote
Gerrit-Change-Id: I118f3559c4647bdd996617443bd371a041711295
Gerrit-Change-Number: 17892
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [rpc] allow reuse of outbound request buffers when retrying
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 06 Oct 2021 00:17:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] [rpc] allow reuse of outbound request buffers when retrying
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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
(2 comments)
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: call_->FreeSidecars();
What about zeroing outbound_sidecars_total_bytes_: should it be done here as well?
http://gerrit.cloudera.org:8080/#/c/17892/2/src/kudu/rpc/rpc_controller.cc@176
PS2, Line 176: return std::move(outbound_sidecars_);
ditto
--
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 <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 01 Oct 2021 19:43:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] [rpc] allow reuse of outbound request buffers when retrying
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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:
(1 comment)
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: controller->Reset();
IIUC, RpcController::Reset() sets outbound_sidecars_total_bytes_ to 0, but it doesn't free sidecars. Is that OK with the rest of the logic?
--
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 <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 01 Oct 2021 19:37:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] [rpc] allow reuse of outbound request buffers when retrying
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 3: Code-Review+2
--
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 <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 07 Oct 2021 23:24:07 +0000
Gerrit-HasComments: No
[kudu-CR] [rpc] allow reuse of outbound request buffers when retrying
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17892
to look at the new patch set (#3).
Change subject: [rpc] allow reuse of outbound request buffers when retrying
......................................................................
[rpc] allow reuse of outbound request buffers when retrying
This patch fixes a heap-use-after-free bug in the address-re-resolving
proxy caused by the fact that requests may be stack allocated, are
serialized once when making the OutboundCall, and then thrown away once
calling the (typically) user-provided callback.
This patch splits out the state that may be used by retries (i.e. the
serialized request, header, and sidecars) into its own class, and passes
this around when retrying.
There are some methods that don't appear to be used in this codebase,
but I opted to keep existing behavior because they seem to be used in
other codebases that share library code. Specifically, I don't see usage
of RpcController::AddOutboundSidecar(), but this does seem to be used in
Impala[1].
1. https://github.com/apache/impala/blob/b28da054f3595bb92873433211438306fc22fbc7/be/src/rpc/sidecar-util.h#L48
Change-Id: I118f3559c4647bdd996617443bd371a041711295
---
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/proxy-test.cc
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/proxy.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
9 files changed, 272 insertions(+), 82 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/17892/3
--
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: newpatchset
Gerrit-Change-Id: I118f3559c4647bdd996617443bd371a041711295
Gerrit-Change-Number: 17892
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)