[kudu-CR] Enable arenas for RPC request and response
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16136 ) Change subject: Enable arenas for RPC request and response .. Enable arenas for RPC request and response This changes the RPC server side to allocate a protobuf Arena for each request. The request RPC and response are allocated from the Arena, ensuring that any sub-messages, strings, repeated fields, etc, use that Arena for allocation as well. Everything is deleted en-masse when the InboundCall object (which owns the Arena) is destructed. This is mostly a straight-forward change except for the change in RaftConsensus. Specifically, we used to do a dirty const_cast to mutate the inbound request and release the ReplicateMsgs, and move them into the raft subsystem. When the request is allocated from an Arena, that 'release' is now actually making a copy, which broke the code path there. Given that there's now a copy happening nonetheless, I just made the code more explicitly construct a new ReplicateMsg copying out of the leader's request. There might be a slight performance degradation here but seemed worth it for code clarity. My assumption here is that anywhere that these copies are substantially expensive we'd probably be disk-bound anyway. Change-Id: I810931900fc2b5f1dec1265abadfb33fb41d29bf Reviewed-on: http://gerrit.cloudera.org:8080/16136 Reviewed-by: Alexey Serbin Tested-by: Todd Lipcon --- M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/rpc/inbound_call.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/service_if.cc 6 files changed, 25 insertions(+), 32 deletions(-) Approvals: Alexey Serbin: Looks good to me, approved Todd Lipcon: Verified -- To view, visit http://gerrit.cloudera.org:8080/16136 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I810931900fc2b5f1dec1265abadfb33fb41d29bf Gerrit-Change-Number: 16136 Gerrit-PatchSet: 6 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Enable arenas for RPC request and response
Todd Lipcon has removed a vote on this change. Change subject: Enable arenas for RPC request and response .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/16136 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I810931900fc2b5f1dec1265abadfb33fb41d29bf Gerrit-Change-Number: 16136 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Enable arenas for RPC request and response
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/16136 ) Change subject: Enable arenas for RPC request and response .. Patch Set 5: Verified+1 The failure here is a pre-existing LSAN issue with openssl, seems unrelated to this patch. -- To view, visit http://gerrit.cloudera.org:8080/16136 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I810931900fc2b5f1dec1265abadfb33fb41d29bf Gerrit-Change-Number: 16136 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 09 Jul 2020 04:33:06 + Gerrit-HasComments: No
[kudu-CR] Enable arenas for RPC request and response
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16136 ) Change subject: Enable arenas for RPC request and response .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16136 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I810931900fc2b5f1dec1265abadfb33fb41d29bf Gerrit-Change-Number: 16136 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 07 Jul 2020 23:25:57 + Gerrit-HasComments: No
[kudu-CR] Enable arenas for RPC request and response
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16136 to look at the new patch set (#5). Change subject: Enable arenas for RPC request and response .. Enable arenas for RPC request and response This changes the RPC server side to allocate a protobuf Arena for each request. The request RPC and response are allocated from the Arena, ensuring that any sub-messages, strings, repeated fields, etc, use that Arena for allocation as well. Everything is deleted en-masse when the InboundCall object (which owns the Arena) is destructed. This is mostly a straight-forward change except for the change in RaftConsensus. Specifically, we used to do a dirty const_cast to mutate the inbound request and release the ReplicateMsgs, and move them into the raft subsystem. When the request is allocated from an Arena, that 'release' is now actually making a copy, which broke the code path there. Given that there's now a copy happening nonetheless, I just made the code more explicitly construct a new ReplicateMsg copying out of the leader's request. There might be a slight performance degradation here but seemed worth it for code clarity. My assumption here is that anywhere that these copies are substantially expensive we'd probably be disk-bound anyway. Change-Id: I810931900fc2b5f1dec1265abadfb33fb41d29bf --- M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/rpc/inbound_call.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/service_if.cc 6 files changed, 25 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/16136/5 -- To view, visit http://gerrit.cloudera.org:8080/16136 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I810931900fc2b5f1dec1265abadfb33fb41d29bf Gerrit-Change-Number: 16136 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Enable arenas for RPC request and response
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/16136 ) Change subject: Enable arenas for RPC request and response .. Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/16136/4/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: http://gerrit.cloudera.org:8080/#/c/16136/4/src/kudu/consensus/raft_consensus.cc@1312 PS4, Line 1312: break > Maybe, return right from here? Done http://gerrit.cloudera.org:8080/#/c/16136/4/src/kudu/consensus/raft_consensus.cc@1317 PS4, Line 1317: RETURN_NOT_OK(s); > If returning non-OK status at line 1312, then drop this ? Done http://gerrit.cloudera.org:8080/#/c/16136/4/src/kudu/rpc/rpc_context.h File src/kudu/rpc/rpc_context.h: http://gerrit.cloudera.org:8080/#/c/16136/4/src/kudu/rpc/rpc_context.h@203 PS4, Line 203: > style nit: stick asterisk to the type ? Done http://gerrit.cloudera.org:8080/#/c/16136/4/src/kudu/rpc/rpc_context.h@204 PS4, Line 204: > ditto Done http://gerrit.cloudera.org:8080/#/c/16136/4/src/kudu/rpc/rpc_context.h@245 PS4, Line 245: const google::protobuf::Message* request_pb_ > Since request_pb_ is never changed, maybe Done http://gerrit.cloudera.org:8080/#/c/16136/4/src/kudu/rpc/rpc_context.h@246 PS4, Line 246: google::protobuf::Message* response_pb_ > Since response_pb_ is never changed, maybe Done http://gerrit.cloudera.org:8080/#/c/16136/4/src/kudu/rpc/service_if.cc File src/kudu/rpc/service_if.cc: http://gerrit.cloudera.org:8080/#/c/16136/4/src/kudu/rpc/service_if.cc@107 PS4, Line 107: CHECK(call->pb_arena()); > When it might ever trigger if InboundCall contains Arena as a member and In just removed it, i think it was leftover from some intermediate version of my patch -- To view, visit http://gerrit.cloudera.org:8080/16136 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I810931900fc2b5f1dec1265abadfb33fb41d29bf Gerrit-Change-Number: 16136 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 07 Jul 2020 22:10:08 + Gerrit-HasComments: Yes
[kudu-CR] Enable arenas for RPC request and response
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16136 ) Change subject: Enable arenas for RPC request and response .. Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/16136/4/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: http://gerrit.cloudera.org:8080/#/c/16136/4/src/kudu/consensus/raft_consensus.cc@1312 PS4, Line 1312: break Maybe, return right from here? http://gerrit.cloudera.org:8080/#/c/16136/4/src/kudu/consensus/raft_consensus.cc@1317 PS4, Line 1317: RETURN_NOT_OK(s); If returning non-OK status at line 1312, then drop this ? http://gerrit.cloudera.org:8080/#/c/16136/4/src/kudu/rpc/rpc_context.h File src/kudu/rpc/rpc_context.h: http://gerrit.cloudera.org:8080/#/c/16136/4/src/kudu/rpc/rpc_context.h@203 PS4, Line 203: style nit: stick asterisk to the type ? http://gerrit.cloudera.org:8080/#/c/16136/4/src/kudu/rpc/rpc_context.h@204 PS4, Line 204: ditto http://gerrit.cloudera.org:8080/#/c/16136/4/src/kudu/rpc/rpc_context.h@245 PS4, Line 245: const google::protobuf::Message* request_pb_ Since request_pb_ is never changed, maybe const google::protobuf::Message* const request_pb_ ? http://gerrit.cloudera.org:8080/#/c/16136/4/src/kudu/rpc/rpc_context.h@246 PS4, Line 246: google::protobuf::Message* response_pb_ Since response_pb_ is never changed, maybe google::protobuf::Message* const response_pb_ ? http://gerrit.cloudera.org:8080/#/c/16136/4/src/kudu/rpc/service_if.cc File src/kudu/rpc/service_if.cc: http://gerrit.cloudera.org:8080/#/c/16136/4/src/kudu/rpc/service_if.cc@107 PS4, Line 107: CHECK(call->pb_arena()); When it might ever trigger if InboundCall contains Arena as a member and InboundCall::pb_arena() is not a virtual method? Could you add a comment to clarify on this? -- To view, visit http://gerrit.cloudera.org:8080/16136 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I810931900fc2b5f1dec1265abadfb33fb41d29bf Gerrit-Change-Number: 16136 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 07 Jul 2020 07:08:40 + Gerrit-HasComments: Yes
[kudu-CR] Enable arenas for RPC request and response
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16136 to look at the new patch set (#4). Change subject: Enable arenas for RPC request and response .. Enable arenas for RPC request and response This changes the RPC server side to allocate a protobuf Arena for each request. The request RPC and response are allocated from the Arena, ensuring that any sub-messages, strings, repeated fields, etc, use that Arena for allocation as well. Everything is deleted en-masse when the InboundCall object (which owns the Arena) is destructed. This is mostly a straight-forward change except for the change in RaftConsensus. Specifically, we used to do a dirty const_cast to mutate the inbound request and release the ReplicateMsgs, and move them into the raft subsystem. When the request is allocated from an Arena, that 'release' is now actually making a copy, which broke the code path there. Given that there's now a copy happening nonetheless, I just made the code more explicitly construct a new ReplicateMsg copying out of the leader's request. There might be a slight performance degradation here but seemed worth it for code clarity. My assumption here is that anywhere that these copies are substantially expensive we'd probably be disk-bound anyway. Change-Id: I810931900fc2b5f1dec1265abadfb33fb41d29bf --- M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/rpc/inbound_call.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/service_if.cc 6 files changed, 24 insertions(+), 27 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/16136/4 -- To view, visit http://gerrit.cloudera.org:8080/16136 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I810931900fc2b5f1dec1265abadfb33fb41d29bf Gerrit-Change-Number: 16136 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] Enable arenas for RPC request and response
Hello Alexey Serbin, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/16136 to review the following change. Change subject: Enable arenas for RPC request and response .. Enable arenas for RPC request and response Change-Id: I810931900fc2b5f1dec1265abadfb33fb41d29bf --- M src/kudu/consensus/consensus_queue-test.cc M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/rpc/inbound_call.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/service_if.cc 7 files changed, 35 insertions(+), 21 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/16136/1 -- To view, visit http://gerrit.cloudera.org:8080/16136 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I810931900fc2b5f1dec1265abadfb33fb41d29bf Gerrit-Change-Number: 16136 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin
