You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2019/12/21 03:42:46 UTC
[kudu-CR] [consensus] respond lock-free to RequestVote() if busy
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14943
Change subject: [consensus] respond lock-free to RequestVote() if busy
......................................................................
[consensus] respond lock-free to RequestVote() if busy
I saw cases of contention on replica's RaftConsensus lock when the
filesystem was slow on updating Raft metadata files to record a vote
that has just been granted by the replica. As it turned out, the fast
path was also acquiring the RaftConsensus object lock, but it's easy
to avoid that.
This patch updates the code by not acquiring the lock in such cases.
It should help a bit with overflowing of the RaftConsensus RPC queue.
In addition, I turned the LOG(INFO) message about this event into
VLOG(1) since it's not so important to report about such events on the
responder side: its state doesn't change upon responding with
ServiceUnavailable anyways.
Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
2 files changed, 40 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/14943/1
--
To view, visit http://gerrit.cloudera.org:8080/14943
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
Gerrit-Change-Number: 14943
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [consensus] respond lock-free to RequestVote() if busy
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14943 )
Change subject: [consensus] respond lock-free to RequestVote() if busy
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/14943
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
Gerrit-Change-Number: 14943
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@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: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 28 Dec 2019 15:00:23 +0000
Gerrit-HasComments: No
[kudu-CR] [consensus] respond lock-free to RequestVote() if busy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14943
to look at the new patch set (#5).
Change subject: [consensus] respond lock-free to RequestVote() if busy
......................................................................
[consensus] respond lock-free to RequestVote() if busy
I saw cases of contention on replica's RaftConsensus lock when the
filesystem was slow on updating Raft metadata files to record a vote
that has just been granted by the replica. As it turned out, the fast
path was also acquiring the RaftConsensus object lock, but it's easy
to avoid that.
This patch updates the code by not acquiring the lock in such cases.
It should help a bit with overflowing of the RaftConsensus RPC queue.
In addition, I turned the LOG(INFO) message about this event into
VLOG(1) since it's not so important to report about such events on the
responder side: its state doesn't change upon responding with
ServiceUnavailable anyways. Other minor clean-up.
Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
2 files changed, 59 insertions(+), 33 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/14943/5
--
To view, visit http://gerrit.cloudera.org:8080/14943
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
Gerrit-Change-Number: 14943
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@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: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [consensus] respond lock-free to RequestVote() if busy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [consensus] respond lock-free to RequestVote() if busy
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/14943
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
Gerrit-Change-Number: 14943
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@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: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [consensus] respond lock-free to RequestVote() if busy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14943 )
Change subject: [consensus] respond lock-free to RequestVote() if busy
......................................................................
Patch Set 5: Verified+1
Unrelated test failure (known issue): https://issues.apache.org/jira/browse/KUDU-1736
--
To view, visit http://gerrit.cloudera.org:8080/14943
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
Gerrit-Change-Number: 14943
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@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: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 28 Dec 2019 00:30:20 +0000
Gerrit-HasComments: No
[kudu-CR] [consensus] respond lock-free to RequestVote() if busy
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14943 )
Change subject: [consensus] respond lock-free to RequestVote() if busy
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14943/2/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
http://gerrit.cloudera.org:8080/#/c/14943/2/src/kudu/consensus/raft_consensus.cc@2342
PS2, Line 2342: FillVoteResponseVoteDenied(ConsensusErrorPB::CONSENSUS_BUSY, response,
: ResponderTermPolicy::CLEAR);
This is a change in behavior (clearing the responder term), isn't it? What makes it safe?
--
To view, visit http://gerrit.cloudera.org:8080/14943
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
Gerrit-Change-Number: 14943
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 24 Dec 2019 04:53:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] [consensus] respond lock-free to RequestVote() if busy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14943 )
Change subject: [consensus] respond lock-free to RequestVote() if busy
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14943/3/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:
http://gerrit.cloudera.org:8080/#/c/14943/3/src/kudu/consensus/raft_consensus.h@586
PS3, Line 586: presrcibed
> typo: prescribed
Done
--
To view, visit http://gerrit.cloudera.org:8080/14943
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
Gerrit-Change-Number: 14943
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@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: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 27 Dec 2019 20:38:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] [consensus] respond lock-free to RequestVote() if busy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14943
to look at the new patch set (#3).
Change subject: [consensus] respond lock-free to RequestVote() if busy
......................................................................
[consensus] respond lock-free to RequestVote() if busy
I saw cases of contention on replica's RaftConsensus lock when the
filesystem was slow on updating Raft metadata files to record a vote
that has just been granted by the replica. As it turned out, the fast
path was also acquiring the RaftConsensus object lock, but it's easy
to avoid that.
This patch updates the code by not acquiring the lock in such cases.
It should help a bit with overflowing of the RaftConsensus RPC queue.
In addition, I turned the LOG(INFO) message about this event into
VLOG(1) since it's not so important to report about such events on the
responder side: its state doesn't change upon responding with
ServiceUnavailable anyways. Other minor clean-up.
Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
2 files changed, 59 insertions(+), 33 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/14943/3
--
To view, visit http://gerrit.cloudera.org:8080/14943
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
Gerrit-Change-Number: 14943
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@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] respond lock-free to RequestVote() if busy
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14943 )
Change subject: [consensus] respond lock-free to RequestVote() if busy
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14943/2/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
http://gerrit.cloudera.org:8080/#/c/14943/2/src/kudu/consensus/raft_consensus.cc@2342
PS2, Line 2342: "replica is already servicing an update from "
: "a current leader or another vote
> If leaving it as-is (i.e. not even clearing) is simpler to grasp, I can pos
Yeah I was asking whether it's safe for us to change this field so that it's no longer set in these responses. From a protobuf standpoint it's fine (as it's optional), but I couldn't really tell whether the rest of the consensus code assumes it is always set.
If you say it's not used by the receiver in these cases, I can buy it.
--
To view, visit http://gerrit.cloudera.org:8080/14943
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
Gerrit-Change-Number: 14943
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@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: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sun, 29 Dec 2019 18:10:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] [consensus] respond lock-free to RequestVote() if busy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14943 )
Change subject: [consensus] respond lock-free to RequestVote() if busy
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14943/2/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
http://gerrit.cloudera.org:8080/#/c/14943/2/src/kudu/consensus/raft_consensus.cc@2342
PS2, Line 2342: FillVoteResponseVoteDenied(ConsensusErrorPB::CONSENSUS_BUSY, response,
: ResponderTermPolicy::CLEAR);
> Yeah I was asking whether it's safe for us to change this field so that it'
As far as I can see, the only benefit of sending the responder's term along with NO vote is about cases described here: https://github.com/apache/kudu/blob/8a36055bc0f96258e006475f33ecff29ab7caee0/src/kudu/consensus/raft_consensus.cc#L2590-L2602
That's a case when there isn't a stable leadership from the point of view of a majority of replicas. The optimization of short-circuited NO vote response is for the case when there is a stable leadership from the responders' point of view.
--
To view, visit http://gerrit.cloudera.org:8080/14943
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
Gerrit-Change-Number: 14943
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@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: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 30 Dec 2019 03:29:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] [consensus] respond lock-free to RequestVote() if busy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14943
to look at the new patch set (#4).
Change subject: [consensus] respond lock-free to RequestVote() if busy
......................................................................
[consensus] respond lock-free to RequestVote() if busy
I saw cases of contention on replica's RaftConsensus lock when the
filesystem was slow on updating Raft metadata files to record a vote
that has just been granted by the replica. As it turned out, the fast
path was also acquiring the RaftConsensus object lock, but it's easy
to avoid that.
This patch updates the code by not acquiring the lock in such cases.
It should help a bit with overflowing of the RaftConsensus RPC queue.
In addition, I turned the LOG(INFO) message about this event into
VLOG(1) since it's not so important to report about such events on the
responder side: its state doesn't change upon responding with
ServiceUnavailable anyways. Other minor clean-up.
Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
2 files changed, 59 insertions(+), 33 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/14943/4
--
To view, visit http://gerrit.cloudera.org:8080/14943
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
Gerrit-Change-Number: 14943
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@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: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [consensus] respond lock-free to RequestVote() if busy
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14943 )
Change subject: [consensus] respond lock-free to RequestVote() if busy
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14943/3/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
http://gerrit.cloudera.org:8080/#/c/14943/3/src/kudu/consensus/raft_consensus.cc@2236
PS3, Line 2236: Substitute("$0Leader $1election vote request",
: LogPrefixThreadSafe(),
: request.is_pre_election() ? "pre-" : "");
> Nit:Same code as Unlocked variant. So perhaps this logic could be a separat
nevermind LogPrefix function is different.
--
To view, visit http://gerrit.cloudera.org:8080/14943
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
Gerrit-Change-Number: 14943
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@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: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 26 Dec 2019 22:41:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] [consensus] respond lock-free to RequestVote() if busy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14943 )
Change subject: [consensus] respond lock-free to RequestVote() if busy
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14943/3/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:
http://gerrit.cloudera.org:8080/#/c/14943/3/src/kudu/consensus/raft_consensus.h@579
PS3, Line 579: DON_NOT_SET
> I suppose you meant DO_NOT_SET
Done
--
To view, visit http://gerrit.cloudera.org:8080/14943
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
Gerrit-Change-Number: 14943
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@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: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 27 Dec 2019 06:39:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] [consensus] respond lock-free to RequestVote() if busy
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14943 )
Change subject: [consensus] respond lock-free to RequestVote() if busy
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14943/3/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:
http://gerrit.cloudera.org:8080/#/c/14943/3/src/kudu/consensus/raft_consensus.h@579
PS3, Line 579: DON_NOT_SET
I suppose you meant DO_NOT_SET
http://gerrit.cloudera.org:8080/#/c/14943/3/src/kudu/consensus/raft_consensus.h@586
PS3, Line 586: presrcibed
typo: prescribed
http://gerrit.cloudera.org:8080/#/c/14943/3/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
http://gerrit.cloudera.org:8080/#/c/14943/3/src/kudu/consensus/raft_consensus.cc@2236
PS3, Line 2236: Substitute("$0Leader $1election vote request",
: LogPrefixThreadSafe(),
: request.is_pre_election() ? "pre-" : "");
Nit:Same code as Unlocked variant. So perhaps this logic could be a separate function.
--
To view, visit http://gerrit.cloudera.org:8080/14943
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
Gerrit-Change-Number: 14943
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@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: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 26 Dec 2019 22:38:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] [consensus] respond lock-free to RequestVote() if busy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14943
to look at the new patch set (#2).
Change subject: [consensus] respond lock-free to RequestVote() if busy
......................................................................
[consensus] respond lock-free to RequestVote() if busy
I saw cases of contention on replica's RaftConsensus lock when the
filesystem was slow on updating Raft metadata files to record a vote
that has just been granted by the replica. As it turned out, the fast
path was also acquiring the RaftConsensus object lock, but it's easy
to avoid that.
This patch updates the code by not acquiring the lock in such cases.
It should help a bit with overflowing of the RaftConsensus RPC queue.
In addition, I turned the LOG(INFO) message about this event into
VLOG(1) since it's not so important to report about such events on the
responder side: its state doesn't change upon responding with
ServiceUnavailable anyways. Other minor clean-up.
Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
2 files changed, 62 insertions(+), 33 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/14943/2
--
To view, visit http://gerrit.cloudera.org:8080/14943
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
Gerrit-Change-Number: 14943
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [consensus] respond lock-free to RequestVote() if busy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14943 )
Change subject: [consensus] respond lock-free to RequestVote() if busy
......................................................................
[consensus] respond lock-free to RequestVote() if busy
I saw cases of contention on replica's RaftConsensus lock when the
filesystem was slow on updating Raft metadata files to record a vote
that has just been granted by the replica. As it turned out, the fast
path was also acquiring the RaftConsensus object lock, but it's easy
to avoid that.
This patch updates the code by not acquiring the lock in such cases.
It should help a bit with overflowing of the RaftConsensus RPC queue.
In addition, I turned the LOG(INFO) message about this event into
VLOG(1) since it's not so important to report about such events on the
responder side: its state doesn't change upon responding with
ServiceUnavailable anyways. Other minor clean-up.
Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
Reviewed-on: http://gerrit.cloudera.org:8080/14943
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Bankim Bhavsar <ba...@cloudera.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
2 files changed, 59 insertions(+), 33 deletions(-)
Approvals:
Alexey Serbin: Verified
Bankim Bhavsar: Looks good to me, but someone else must approve
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/14943
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
Gerrit-Change-Number: 14943
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@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: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [consensus] respond lock-free to RequestVote() if busy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14943 )
Change subject: [consensus] respond lock-free to RequestVote() if busy
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14943/2/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
http://gerrit.cloudera.org:8080/#/c/14943/2/src/kudu/consensus/raft_consensus.cc@2342
PS2, Line 2342: FillVoteResponseVoteDenied(ConsensusErrorPB::CONSENSUS_BUSY, response,
: ResponderTermPolicy::CLEAR);
> This is a change in behavior (clearing the responder term), isn't it? What
If leaving it as-is (i.e. not even clearing) is simpler to grasp, I can post a patch simply not touch the field.
Or your question is about whether it's safe to not even set the repsponder's term? If the latter, I'm going to post a clean-up patch for the leader election piece with assertions on having the term in places where it's expected to arrive.
Continuing the latter theme, the semantics of the responder_term assumes the receiver of the response can make some sense of that field. Current code isn't using it at all (and if I'm not mistaken the Raft spec doesn't require it to be set when responding with NO vote).
--
To view, visit http://gerrit.cloudera.org:8080/14943
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95d5cbe455fefc4cdc540ee1e7b69e1f21b6ebc0
Gerrit-Change-Number: 14943
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@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: Tue, 24 Dec 2019 05:52:09 +0000
Gerrit-HasComments: Yes