You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2019/10/19 00:14:38 UTC
[kudu-CR] consensus peers: make RpcPeerProxy::StartElection async
Hello Alexey Serbin, Andrew Wong,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/14512
to review the following change.
Change subject: consensus_peers: make RpcPeerProxy::StartElection async
......................................................................
consensus_peers: make RpcPeerProxy::StartElection async
It was the only such proxy call that was synchronous. Changing it means
adding some stupid code to Peer::StartElection, but I think it's still a net
improvement in clarity. Along the way I also did some other C++11 cleanup.
Change-Id: I6a6df610f1c07adae5a85534d8c6dec324801042
---
M src/kudu/consensus/consensus-test-util.h
M src/kudu/consensus/consensus_peers.cc
M src/kudu/consensus/consensus_peers.h
M src/kudu/consensus/leader_election.cc
M src/kudu/consensus/peer_manager.cc
5 files changed, 134 insertions(+), 112 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/14512/1
--
To view, visit http://gerrit.cloudera.org:8080/14512
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6a6df610f1c07adae5a85534d8c6dec324801042
Gerrit-Change-Number: 14512
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
[kudu-CR] consensus peers: make RpcPeerProxy::StartElection async and other cleanup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14512 )
Change subject: consensus_peers: make RpcPeerProxy::StartElection async and other cleanup
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14512/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14512/1//COMMIT_MSG@8
PS1, Line 8:
: It was the only such proxy call that was synchronous.
> Thanks for the context; could you briefly summarize the utility of this pat
I think I overstated the effect of the patch in my original response, but nevertheless I'll update the commit message with more color.
--
To view, visit http://gerrit.cloudera.org:8080/14512
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a6df610f1c07adae5a85534d8c6dec324801042
Gerrit-Change-Number: 14512
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 21 Oct 2019 05:53:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] consensus peers: make RpcPeerProxy::StartElection async
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14512 )
Change subject: consensus_peers: make RpcPeerProxy::StartElection async
......................................................................
Patch Set 1:
(1 comment)
Overall looks good to me, just a few nits.
http://gerrit.cloudera.org:8080/#/c/14512/1/src/kudu/consensus/peer_manager.cc
File src/kudu/consensus/peer_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14512/1/src/kudu/consensus/peer_manager.cc@110
PS1, Line 110: Status PeerManager::StartElection(const std::string& uuid) {
This method is called from raft_consensus.cc, now now the warning in raft_consensus.cc:986 is useless.
Does it make sense to move the warning from there into the callback code?
--
To view, visit http://gerrit.cloudera.org:8080/14512
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a6df610f1c07adae5a85534d8c6dec324801042
Gerrit-Change-Number: 14512
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 19 Oct 2019 00:36:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] consensus peers: make RpcPeerProxy::StartElection async and other cleanup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14512
to look at the new patch set (#2).
Change subject: consensus_peers: make RpcPeerProxy::StartElection async and other cleanup
......................................................................
consensus_peers: make RpcPeerProxy::StartElection async and other cleanup
It was the only such proxy call that was synchronous. Apart from violating
POLA, the synchronicity meant that the graceful leadership transfer code
path occupied a Raft threadpool slot for longer in order to receive the RPC
response. It's not a hot path so this is by no means a perf improvement, but
it just seemed unnecessary given that it was only used to log a warning in
the rare event that the remote couldn't start an election.
Changing this meant adding some extra lifecycle code to Peer::StartElection,
but I think it's still a net improvement in clarity.
Along the way I also did some other C++11 cleanup.
Change-Id: I6a6df610f1c07adae5a85534d8c6dec324801042
---
M src/kudu/consensus/consensus-test-util.h
M src/kudu/consensus/consensus_peers.cc
M src/kudu/consensus/consensus_peers.h
M src/kudu/consensus/leader_election.cc
M src/kudu/consensus/peer_manager.cc
5 files changed, 134 insertions(+), 112 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/14512/2
--
To view, visit http://gerrit.cloudera.org:8080/14512
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6a6df610f1c07adae5a85534d8c6dec324801042
Gerrit-Change-Number: 14512
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] consensus peers: make RpcPeerProxy::StartElection async and other cleanup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14512 )
Change subject: consensus_peers: make RpcPeerProxy::StartElection async and other cleanup
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14512/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14512/2//COMMIT_MSG@11
PS2, Line 11: for longer in order to receive the RPC
: response.
> Just for my own understanding, the assumptions here are that:
Correct. To add to #2, just as we didn't care about the result, we didn't care if the RPC failed to send either.
--
To view, visit http://gerrit.cloudera.org:8080/14512
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a6df610f1c07adae5a85534d8c6dec324801042
Gerrit-Change-Number: 14512
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 21 Oct 2019 06:56:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] consensus peers: make RpcPeerProxy::StartElection async
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14512 )
Change subject: consensus_peers: make RpcPeerProxy::StartElection async
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14512/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14512/1//COMMIT_MSG@8
PS1, Line 8:
: It was the only such proxy call that was synchronous.
> Is there a functional reason for making the change? Alone, this patch doesn
I noticed this while reviewing https://gerrit.cloudera.org/c/14420 and trying to wrap my head around RpcPeerProxy's lifecycle requirements. The fact that StartElection was the only synchronous method bothered me, and I also thought it was unnecessary for the graceful leadership transfer path to run a task on the Raft thread pool that waited for an RPC response, only so it could emit a warning if the RPC failed.
Agreed that the heap allocation is a frustrating side effect, though this isn't a hot path. If you think the result is net worse code I can strip that part of the patch.
--
To view, visit http://gerrit.cloudera.org:8080/14512
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a6df610f1c07adae5a85534d8c6dec324801042
Gerrit-Change-Number: 14512
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 21 Oct 2019 04:40:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] consensus peers: make RpcPeerProxy::StartElection async
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14512 )
Change subject: consensus_peers: make RpcPeerProxy::StartElection async
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14512/1/src/kudu/consensus/peer_manager.cc
File src/kudu/consensus/peer_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14512/1/src/kudu/consensus/peer_manager.cc@110
PS1, Line 110: Status PeerManager::StartElection(const std::string& uuid) {
> The warning is still somewhat useful though; we can return a bad Status on
Ah, indeed.
--
To view, visit http://gerrit.cloudera.org:8080/14512
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a6df610f1c07adae5a85534d8c6dec324801042
Gerrit-Change-Number: 14512
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 19 Oct 2019 02:25:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] consensus peers: make RpcPeerProxy::StartElection async and other cleanup
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14512 )
Change subject: consensus_peers: make RpcPeerProxy::StartElection async and other cleanup
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14512
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a6df610f1c07adae5a85534d8c6dec324801042
Gerrit-Change-Number: 14512
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 21 Oct 2019 17:29:43 +0000
Gerrit-HasComments: No
[kudu-CR] consensus peers: make RpcPeerProxy::StartElection async and other cleanup
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14512 )
Change subject: consensus_peers: make RpcPeerProxy::StartElection async and other cleanup
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14512/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14512/2//COMMIT_MSG@11
PS2, Line 11: for longer in order to receive the RPC
: response.
Just for my own understanding, the assumptions here are that:
1. we never cared about the result -- we just log a warning, and
2. we now don't care about waiting to receive the response (and frankly, we don't care if the RPC failed to send) by the time we finish calling StartElection().
Are those correct?
--
To view, visit http://gerrit.cloudera.org:8080/14512
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a6df610f1c07adae5a85534d8c6dec324801042
Gerrit-Change-Number: 14512
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 21 Oct 2019 06:17:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] consensus peers: make RpcPeerProxy::StartElection async
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14512 )
Change subject: consensus_peers: make RpcPeerProxy::StartElection async
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14512/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14512/1//COMMIT_MSG@8
PS1, Line 8:
: It was the only such proxy call that was synchronous.
> I noticed this while reviewing https://gerrit.cloudera.org/c/14420 and tryi
Thanks for the context; could you briefly summarize the utility of this patch in the commit message w.r.t that it now means we don't have to run elections on the Raft threadpool? And also what it means for graceful leadership transfer?
I do think it's less straight-forward code, but if it unlocks some new functionality, and you've thought through that we're not now breaking any invariants w.r.t the completion of StartElection() calls, I'm fine with that.
--
To view, visit http://gerrit.cloudera.org:8080/14512
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a6df610f1c07adae5a85534d8c6dec324801042
Gerrit-Change-Number: 14512
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 21 Oct 2019 05:15:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] consensus peers: make RpcPeerProxy::StartElection async and other cleanup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14512 )
Change subject: consensus_peers: make RpcPeerProxy::StartElection async and other cleanup
......................................................................
consensus_peers: make RpcPeerProxy::StartElection async and other cleanup
It was the only such proxy call that was synchronous. Apart from violating
POLA, the synchronicity meant that the graceful leadership transfer code
path occupied a Raft threadpool slot for longer in order to receive the RPC
response. It's not a hot path so this is by no means a perf improvement, but
it just seemed unnecessary given that it was only used to log a warning in
the rare event that the remote couldn't start an election.
Changing this meant adding some extra lifecycle code to Peer::StartElection,
but I think it's still a net improvement in clarity.
Along the way I also did some other C++11 cleanup.
Change-Id: I6a6df610f1c07adae5a85534d8c6dec324801042
Reviewed-on: http://gerrit.cloudera.org:8080/14512
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/consensus/consensus-test-util.h
M src/kudu/consensus/consensus_peers.cc
M src/kudu/consensus/consensus_peers.h
M src/kudu/consensus/leader_election.cc
M src/kudu/consensus/peer_manager.cc
5 files changed, 134 insertions(+), 112 deletions(-)
Approvals:
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/14512
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6a6df610f1c07adae5a85534d8c6dec324801042
Gerrit-Change-Number: 14512
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] consensus peers: make RpcPeerProxy::StartElection async
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14512 )
Change subject: consensus_peers: make RpcPeerProxy::StartElection async
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14512/1/src/kudu/consensus/peer_manager.cc
File src/kudu/consensus/peer_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14512/1/src/kudu/consensus/peer_manager.cc@110
PS1, Line 110: Status PeerManager::StartElection(const std::string& uuid) {
> This method is called from raft_consensus.cc, now now the warning in raft_c
The warning is still somewhat useful though; we can return a bad Status on L117 here.
--
To view, visit http://gerrit.cloudera.org:8080/14512
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a6df610f1c07adae5a85534d8c6dec324801042
Gerrit-Change-Number: 14512
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 19 Oct 2019 00:52:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] consensus peers: make RpcPeerProxy::StartElection async
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14512 )
Change subject: consensus_peers: make RpcPeerProxy::StartElection async
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14512/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14512/1//COMMIT_MSG@8
PS1, Line 8:
: It was the only such proxy call that was synchronous.
Is there a functional reason for making the change? Alone, this patch doesn't seem to do much, though now a bunch of stuff is allocated on heap and some non-C++14 code.
--
To view, visit http://gerrit.cloudera.org:8080/14512
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a6df610f1c07adae5a85534d8c6dec324801042
Gerrit-Change-Number: 14512
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 19 Oct 2019 02:47:12 +0000
Gerrit-HasComments: Yes