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 2017/09/25 22:45:53 UTC
[kudu-CR] WIP [consensus] introduce adding NON VOTER members
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8138
Change subject: WIP [consensus] introduce adding NON_VOTER members
......................................................................
WIP [consensus] introduce adding NON_VOTER members
Added ability to add NON_VOTER member replicas into Raft configuration.
Updated the kudu CLI accordingly. Also, added a new integration test:
RaftConsensusITest.AddNonVoterReplica.
WIP: I'd like to get some feedback on the update of the RaftPeerPB.
Also, the leader is about to fill in the timestamp in every
new element of the 'intended_operations' field. Also, I need
to add more tests.
Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
---
M src/kudu/consensus/metadata.proto
M src/kudu/consensus/quorum_util.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/tools/tool_action_tablet.cc
5 files changed, 163 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/8138/1
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] WIP [consensus] introduce adding NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8138 )
Change subject: WIP [consensus] introduce adding NON_VOTER members
......................................................................
Patch Set 2:
> Curious how the client(s) will respond to seeing a non-voter. Does
> the master report non-voters to clients when they request tablet
> locations, and if so, do the clients know that they can use
> non-voters as readable replicas?
It's a good question and I'm planning to add a test for that as well (that item is mentioned in the replication improvement design doc). The idea is that the client should not see those non-voters if using currently established Kudu client API. I'm adding a test for that right now.
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 27 Sep 2017 17:45:47 +0000
Gerrit-HasComments: No
[kudu-CR] [consensus] adding/removing NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/8138 )
Change subject: [consensus] adding/removing NON_VOTER members
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 15
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP [consensus] introduce adding NON VOTER members
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8138 )
Change subject: WIP [consensus] introduce adding NON_VOTER members
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8138/2/src/kudu/integration-tests/raft_consensus-itest.cc
File src/kudu/integration-tests/raft_consensus-itest.cc:
http://gerrit.cloudera.org:8080/#/c/8138/2/src/kudu/integration-tests/raft_consensus-itest.cc@416
PS2, Line 416: // Get number of source tablet copy sessions at the specified server.
nit: leave one blank line above this, for consistency.
http://gerrit.cloudera.org:8080/#/c/8138/2/src/kudu/integration-tests/raft_consensus-itest.cc@3242
PS2, Line 3242: TEST_F(RaftConsensusITest, AddNonVoterReplica) {
I think this is a good addition, but that, ultimately we also need to add non_voter replicas to a lot of the other full blown integration tests. possible candidates from the most hardened/tricky ones I'd say are *CrashyNodes* *ChurnyElections* in this itest and linked-list-test.
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 26 Sep 2017 20:56:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] [consensus] adding/removing NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8138
to look at the new patch set (#15).
Change subject: [consensus] adding/removing NON_VOTER members
......................................................................
[consensus] adding/removing NON_VOTER members
Added ability to add and remove NON_VOTER member replicas.
Updated the kudu CLI accordingly. Also, added new integration test:
* RaftConsensusITest.AddNonVoterReplica
* RaftConsensusITest.AddThenRemoveNonVoterReplica
* RaftConsensusITest.NonVoterReplicasDoNotVote
Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
---
M src/kudu/consensus/leader_election-test.cc
M src/kudu/consensus/leader_election.cc
M src/kudu/consensus/leader_election.h
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/quorum_util.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/raft_consensus-itest-base.cc
M src/kudu/integration-tests/raft_consensus-itest-base.h
M src/kudu/integration-tests/raft_consensus-itest.cc
A src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
13 files changed, 671 insertions(+), 82 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/8138/15
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 15
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP [consensus] adding/removing NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8138
to look at the new patch set (#7).
Change subject: WIP [consensus] adding/removing NON_VOTER members
......................................................................
WIP [consensus] adding/removing NON_VOTER members
Added ability to add and remove NON_VOTER member replicas.
Updated the kudu CLI accordingly. Also, added new integration test:
* RaftConsensusITest.AddNonVoterReplica
* RaftConsensusITest.AddThenRemoveNonVoterReplica
* RaftConsensusITest.NonVoterReplicasDoNotVote
WIP: Perhaps, I should continue gathering feedback on this patch
and adding more tests.
Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
---
M src/kudu/consensus/leader_election-test.cc
M src/kudu/consensus/leader_election.cc
M src/kudu/consensus/leader_election.h
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
8 files changed, 534 insertions(+), 58 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/8138/7
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP [consensus] adding/removing NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8138 )
Change subject: WIP [consensus] adding/removing NON_VOTER members
......................................................................
Patch Set 7:
(19 comments)
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/leader_election.cc
File src/kudu/consensus/leader_election.cc:
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/leader_election.cc@167
PS6, Line 167: LOG_WITH_PREFIX(WARNING) << Substitute(
> I thought about that, but decided that would be too much: if due do some bu
Oh, I take it back. It seemed to me the RequestVotePB parameter arrives from some other peer, but that's not the case: RequestVotePB is generated by current peer and passed as a parameter into this constructor.
So, after some consideration I think DCHECK() would be the best option here.
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/consensus/leader_election.cc
File src/kudu/consensus/leader_election.cc:
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/consensus/leader_election.cc@166
PS7, Line 166: // Only voters should start leader elections.
> should this be a DCHECK instead of just a warning? wouldn't it indicate a b
Right, Mike pointed at that already, but it seemed to me that this LeaderElection() constructor was called not only to start an election, but also to process VoteRequestPB when such a request arrives from a peer. I found it was wrong, so I'm adding that DCHECK() here.
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/consensus/leader_election.cc@189
PS7, Line 189: Voters
> nit: "Voter UUIDs" reads better for me
Done
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/consensus/quorum_util-test.cc
File src/kudu/consensus/quorum_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/consensus/quorum_util-test.cc@33
PS7, Line 33: The membership type
: // is optional: if not specified, the membership type is set to VOTER
> it doesn't look optional to me
Yep, the comment is stale. Fixed.
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/consensus/quorum_util-test.cc@173
PS7, Line 173: nmi_peer_uuid
> what does 'nmi' stand for?
nmi -- 'no member_type info'. agreed, that's not expressive one, will update.
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/raft_consensus.cc@410
PS6, Line 410: DisableFailureDetector();
> I'm not sure about that.
Ah, regarding DisableFailureDetector() I think you are right -- that might be better.
The second part of my comment was about returning IllegalState() status, actually.
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/consensus/raft_consensus.cc@405
PS7, Line 405: if (PREDICT_FALSE(active_role != RaftPeerPB::LEADER &&
> isn't this condition guaranteed to be true because of the if statement just
Right, but it's about voter-related role in general. I replaced that with IsVoterRole() to be less confusing.
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/consensus/raft_consensus.cc@405
PS7, Line 405: active_role != RaftPeerPB::LEADER &&
: active_role != RaftPeerPB::FOLLOWER
> what about adding a utility function like DoesRoleVote(active_role) in quor
That's a good idea. Done.
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/consensus/raft_consensus.cc@410
PS7, Line 410: DisableFailureDetector();
> I'm not sure this line is necessary. Shouldn't we have never enabled it in
Since we start the failure detection upon start of every replica, it's necessary to do that somewhere. I didn't find the place where things are done in a centralized way upon configuration change.
OK, I need to introduce that for generic configuration change.
No tests failed when removing this, but that's expected since in that case StartElection() return an error and in the log there are multiple messages like 'failed to trigger leader election: Illegal state: only voting members can start elections'.
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc
File src/kudu/integration-tests/raft_consensus-itest.cc:
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc@3286
PS7, Line 3286: tablet cluster
> I think just 'tablet' is clearer
Done
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc@3293
PS7, Line 3293: it
> nit: its
Done
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc@3296
PS7, Line 3296: // * Tablet leader is established and it reports about the newly added replica
> isn't this already done as soon as the config change is committed? the comp
Right, committing Raft configuration update means the leader has been already established.
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc@3299
PS7, Line 3299: and
> 'a new one'
Done
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc@3300
PS7, Line 3300: tablet cluster is available
> not clear what this means. You mean that the 'updated leader is available a
as the sentence explains, that means it's possible to insert data into corresponding table. Updated.
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc@3301
PS7, Line 3301: is hosted by
> contains
Done
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc@3378
PS7, Line 3378: GetTabletCopyTgtSessionsCount
> let's not abbreviate 'Target' here
Done
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc@3410
PS7, Line 3410: ASSERT_OK(LeaderStepDown(leader, tablet_id, kTimeout));
> add a comment for this block here explaining why you're asking it to step d
Done. Yep, that's about checking that the update cluster is able to elect a leader.
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc@3423
PS7, Line 3423: // Ensure that the replicas converge.
> curious: does this test infrastructure also poll the NON_VOTER replica to s
Along with other verification steps, ClusterVerifier employs VerifyCommittedOpIdsMatch() which works with local files under tablet servers to verify that OpIds for all existent tablets match. In that sense, NON_VOTER replicas are covered as well, yes.
I added corresponding comment.
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc@3507
PS7, Line 3507: // Remove the newly added replica.
> can we also assert here that the removed replica gets TOMBSTONED?
That's a good idea, thanks!
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 06 Oct 2017 23:51:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [consensus] adding/removing NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8138
to look at the new patch set (#3).
Change subject: WIP [consensus] adding/removing NON_VOTER members
......................................................................
WIP [consensus] adding/removing NON_VOTER members
Added ability to add and remove NON_VOTER member replicas.
Updated the kudu CLI accordingly. Also, added new integration test:
* RaftConsensusITest.AddNonVoterReplica
* RaftConsensusITest.AddThenRemoveNonVoterReplica
WIP: Perhaps, I should continue gathering feedback on this patch
and adding more tests.
Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
---
M src/kudu/consensus/leader_election.cc
M src/kudu/consensus/leader_election.h
M src/kudu/consensus/quorum_util.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/master/catalog_manager.cc
6 files changed, 264 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/8138/3
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [consensus] adding/removing NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8138 )
Change subject: [consensus] adding/removing NON_VOTER members
......................................................................
[consensus] adding/removing NON_VOTER members
Added ability to add and remove NON_VOTER member replicas.
Updated the kudu CLI accordingly. Also, added new integration test:
* RaftConsensusITest.AddNonVoterReplica
* RaftConsensusITest.AddThenRemoveNonVoterReplica
* RaftConsensusITest.NonVoterReplicasDoNotVote
Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Reviewed-on: http://gerrit.cloudera.org:8080/8138
Reviewed-by: Todd Lipcon <to...@apache.org>
Reviewed-by: Mike Percy <mp...@apache.org>
Tested-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/consensus/leader_election-test.cc
M src/kudu/consensus/leader_election.cc
M src/kudu/consensus/leader_election.h
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/quorum_util.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/raft_consensus-itest-base.cc
M src/kudu/integration-tests/raft_consensus-itest-base.h
M src/kudu/integration-tests/raft_consensus-itest.cc
A src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
13 files changed, 671 insertions(+), 82 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Mike Percy: Looks good to me, approved
Alexey Serbin: Verified
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 16
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [consensus] adding/removing NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Adar Dembo from this change. ( http://gerrit.cloudera.org:8080/8138 )
Change subject: [consensus] adding/removing NON_VOTER members
......................................................................
Removed reviewer Adar Dembo.
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 15
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP [consensus] adding/removing NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8138 )
Change subject: WIP [consensus] adding/removing NON_VOTER members
......................................................................
Patch Set 7:
> (8 comments)
>
> This is looking pretty good although I have some comments. I don't
> think we need to add client tool support to this patch before
> merging it, since it simply adds infrastructure and features that
> are not possible to activate without extraordinary effort.
Thank you for the review.
Yes, agree that client tool support should be a separate patch. Actually, I already created a separate patch to introduce replica selector: https://gerrit.cloudera.org/#/c/8161/ I'm basing my patch for kudu CLI tool changes based on that.
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 05 Oct 2017 18:55:09 +0000
Gerrit-HasComments: No
[kudu-CR] WIP [consensus] introduce adding NON VOTER members
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8138 )
Change subject: WIP [consensus] introduce adding NON_VOTER members
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8138/1/src/kudu/consensus/metadata.proto
File src/kudu/consensus/metadata.proto:
http://gerrit.cloudera.org:8080/#/c/8138/1/src/kudu/consensus/metadata.proto@42
PS1, Line 42: // Indicates that this node participates in the configuration in a passive role,
: // i.e. that it accepts Consensus::Update() calls but does not participate
: // in elections or majorities.
: LEARNER = 2;
these seem redundant, should we so something about it?
in code I can only find that VOTER == FOLLOWER and that any other member type {NON_VOTER, UNKNOWN_MEMBER_TYPE} == NON_VOTER
http://gerrit.cloudera.org:8080/#/c/8138/1/src/kudu/consensus/quorum_util.cc
File src/kudu/consensus/quorum_util.cc:
http://gerrit.cloudera.org:8080/#/c/8138/1/src/kudu/consensus/quorum_util.cc@175
PS1, Line 175: A non-voter member must have intended operations present: by
: // definition, a non-voter member is in transitional state, and its Raft
: // metadata stores the information on the planned transition.
I thought the fact of whether we'd have peers have "intentions" was still under discussion in the doc. maybe I misunderstood.
In any case maybe in the milestones this work has ahead we don't need to have intentions from get go (even if we end up using them)?
We could start by having RPCs (and thus tools) to:
- add new non_voter replica
- remove non_voter replica
- promote non_voter replica
- demote non_voter replica
and only then do the "intentions" bit?
wdyt?
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 25 Sep 2017 23:01:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] [consensus] adding/removing NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8138
to look at the new patch set (#9).
Change subject: [consensus] adding/removing NON_VOTER members
......................................................................
[consensus] adding/removing NON_VOTER members
Added ability to add and remove NON_VOTER member replicas.
Updated the kudu CLI accordingly. Also, added new integration test:
* RaftConsensusITest.AddNonVoterReplica
* RaftConsensusITest.AddThenRemoveNonVoterReplica
* RaftConsensusITest.NonVoterReplicasDoNotVote
Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
---
M src/kudu/consensus/leader_election-test.cc
M src/kudu/consensus/leader_election.cc
M src/kudu/consensus/leader_election.h
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/quorum_util.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
9 files changed, 557 insertions(+), 58 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/8138/9
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 9
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP [consensus] introduce adding NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8138 )
Change subject: WIP [consensus] introduce adding NON_VOTER members
......................................................................
Patch Set 1:
(3 comments)
Thank you for the review.
I'll add the missing tests and CLI support for promote/demote operations.
http://gerrit.cloudera.org:8080/#/c/8138/1/src/kudu/consensus/metadata.proto
File src/kudu/consensus/metadata.proto:
http://gerrit.cloudera.org:8080/#/c/8138/1/src/kudu/consensus/metadata.proto@42
PS1, Line 42: // Indicates that this node participates in the configuration in a passive role,
: // i.e. that it accepts Consensus::Update() calls but does not participate
: // in elections or majorities.
: LEARNER = 2;
> actually make that VOTER == {LEADER, FOLLOWER} so maybe it does make sense
Yup, I think it's just some other projection of the information in the cluster.
I think we can keep them for a while, at least, I would create a separate changelist to remove those, if I would.
http://gerrit.cloudera.org:8080/#/c/8138/1/src/kudu/consensus/quorum_util.cc
File src/kudu/consensus/quorum_util.cc:
http://gerrit.cloudera.org:8080/#/c/8138/1/src/kudu/consensus/quorum_util.cc@175
PS1, Line 175: A non-voter member must have intended operations present: by
: // definition, a non-voter member is in transitional state, and its Raft
: // metadata stores the information on the planned transition.
> I thought the fact of whether we'd have peers have "intentions" was still u
Yes, I think we can make it YAGNI-style and postpone adding the fields only when we are about to add code which uses them (e.g., when we need to estimate the progress of the catch-up process). Especially given the fact that we are about to have two-step process there, i.e. the the code which would rely on the 'intended' operations is going to appear later.
http://gerrit.cloudera.org:8080/#/c/8138/1/src/kudu/integration-tests/raft_consensus-itest.cc
File src/kudu/integration-tests/raft_consensus-itest.cc:
http://gerrit.cloudera.org:8080/#/c/8138/1/src/kudu/integration-tests/raft_consensus-itest.cc@106
PS1, Line 106: using std::set;
> warning: using decl 'set' is unused [misc-unused-using-decls]
Done
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 25 Sep 2017 23:55:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [consensus] adding/removing NON VOTER members
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8138 )
Change subject: WIP [consensus] adding/removing NON_VOTER members
......................................................................
Patch Set 7:
(17 comments)
Looks pretty good. I agree we don't need to include CLI tools into this same patch - it's already 500+ lines, no need to make it bigger :)
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/consensus/leader_election.cc
File src/kudu/consensus/leader_election.cc:
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/consensus/leader_election.cc@166
PS7, Line 166: // Only voters should start leader elections.
should this be a DCHECK instead of just a warning? wouldn't it indicate a bug if we tried to start a leader-election as a non-voter?
seems like we already have similar checks in RaftConsensus::StartElection which is where it might be more appropriate to double-check that we are an appropriate role. (we already check for NON_PARTICIPANT)
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/consensus/leader_election.cc@189
PS7, Line 189: Voters
nit: "Voter UUIDs" reads better for me
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/consensus/quorum_util-test.cc
File src/kudu/consensus/quorum_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/consensus/quorum_util-test.cc@33
PS7, Line 33: The membership type
: // is optional: if not specified, the membership type is set to VOTER
it doesn't look optional to me
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/consensus/quorum_util-test.cc@173
PS7, Line 173: nmi_peer_uuid
what does 'nmi' stand for?
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/consensus/raft_consensus.cc@405
PS7, Line 405: if (PREDICT_FALSE(active_role != RaftPeerPB::LEADER &&
isn't this condition guaranteed to be true because of the if statement just above?
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/consensus/raft_consensus.cc@405
PS7, Line 405: active_role != RaftPeerPB::LEADER &&
: active_role != RaftPeerPB::FOLLOWER
what about adding a utility function like DoesRoleVote(active_role) in quorum_util so that this is centalized?
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/consensus/raft_consensus.cc@410
PS7, Line 410: DisableFailureDetector();
I'm not sure this line is necessary. Shouldn't we have never enabled it in the first place, in which case the only reason we'd get to StartElection is from a remote request or from some kind of delayed callback across a term switch?
I'm curious: If you remove this line do any tests fail?
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc
File src/kudu/integration-tests/raft_consensus-itest.cc:
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc@3286
PS7, Line 3286: tablet cluster
I think just 'tablet' is clearer
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc@3293
PS7, Line 3293: it
nit: its
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc@3296
PS7, Line 3296: // * Tablet leader is established and it reports about the newly added replica
isn't this already done as soon as the config change is committed? the completion of the tablet copy should have no bearing on the leader election or reporting of the config change to the master.
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc@3299
PS7, Line 3299: and
'a new one'
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc@3300
PS7, Line 3300: tablet cluster is available
not clear what this means. You mean that the 'updated leader is available at the master'?
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc@3301
PS7, Line 3301: is hosted by
contains
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc@3378
PS7, Line 3378: GetTabletCopyTgtSessionsCount
let's not abbreviate 'Target' here
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc@3410
PS7, Line 3410: ASSERT_OK(LeaderStepDown(leader, tablet_id, kTimeout));
add a comment for this block here explaining why you're asking it to step down? (it's just verifying that it can re-elect?)
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc@3423
PS7, Line 3423: // Ensure that the replicas converge.
curious: does this test infrastructure also poll the NON_VOTER replica to see that it is up to date?
http://gerrit.cloudera.org:8080/#/c/8138/7/src/kudu/integration-tests/raft_consensus-itest.cc@3507
PS7, Line 3507: // Remove the newly added replica.
can we also assert here that the removed replica gets TOMBSTONED?
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 06 Oct 2017 01:02:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [consensus] introduce adding NON VOTER members
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8138 )
Change subject: WIP [consensus] introduce adding NON_VOTER members
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8138/1/src/kudu/consensus/metadata.proto
File src/kudu/consensus/metadata.proto:
http://gerrit.cloudera.org:8080/#/c/8138/1/src/kudu/consensus/metadata.proto@42
PS1, Line 42: // Indicates that this node participates in the configuration in a passive role,
: // i.e. that it accepts Consensus::Update() calls but does not participate
: // in elections or majorities.
: LEARNER = 2;
> these seem redundant, should we so something about it?
actually make that VOTER == {LEADER, FOLLOWER} so maybe it does make sense to keep them
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 25 Sep 2017 23:02:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] [consensus] adding/removing NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8138
to look at the new patch set (#14).
Change subject: [consensus] adding/removing NON_VOTER members
......................................................................
[consensus] adding/removing NON_VOTER members
Added ability to add and remove NON_VOTER member replicas.
Updated the kudu CLI accordingly. Also, added new integration test:
* RaftConsensusITest.AddNonVoterReplica
* RaftConsensusITest.AddThenRemoveNonVoterReplica
* RaftConsensusITest.NonVoterReplicasDoNotVote
Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
---
M src/kudu/consensus/leader_election-test.cc
M src/kudu/consensus/leader_election.cc
M src/kudu/consensus/leader_election.h
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/quorum_util.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/raft_consensus-itest-base.cc
M src/kudu/integration-tests/raft_consensus-itest-base.h
M src/kudu/integration-tests/raft_consensus-itest.cc
A src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
13 files changed, 673 insertions(+), 83 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/8138/14
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 14
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [consensus] adding/removing NON VOTER members
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8138 )
Change subject: [consensus] adding/removing NON_VOTER members
......................................................................
Patch Set 11: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 16 Oct 2017 05:13:41 +0000
Gerrit-HasComments: No
[kudu-CR] WIP [consensus] adding/removing NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8138
to look at the new patch set (#5).
Change subject: WIP [consensus] adding/removing NON_VOTER members
......................................................................
WIP [consensus] adding/removing NON_VOTER members
Added ability to add and remove NON_VOTER member replicas.
Updated the kudu CLI accordingly. Also, added new integration test:
* RaftConsensusITest.AddNonVoterReplica
* RaftConsensusITest.AddThenRemoveNonVoterReplica
WIP: Perhaps, I should continue gathering feedback on this patch
and adding more tests.
Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
---
M src/kudu/consensus/leader_election-test.cc
M src/kudu/consensus/leader_election.cc
M src/kudu/consensus/leader_election.h
M src/kudu/consensus/metadata.proto
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
9 files changed, 431 insertions(+), 58 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/8138/5
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [consensus] adding/removing NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8138 )
Change subject: [consensus] adding/removing NON_VOTER members
......................................................................
Patch Set 11:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/8138/8/src/kudu/consensus/leader_election.cc
File src/kudu/consensus/leader_election.cc:
http://gerrit.cloudera.org:8080/#/c/8138/8/src/kudu/consensus/leader_election.cc@166
PS8, Line 166: tries
> tried
Done
http://gerrit.cloudera.org:8080/#/c/8138/11/src/kudu/integration-tests/raft_consensus-itest.cc
File src/kudu/integration-tests/raft_consensus-itest.cc:
http://gerrit.cloudera.org:8080/#/c/8138/11/src/kudu/integration-tests/raft_consensus-itest.cc@204
PS11, Line 204: Status GetTabletCopySourceSessionsCount(const ExternalDaemon& server,
> unimplemented
Done
http://gerrit.cloudera.org:8080/#/c/8138/11/src/kudu/integration-tests/raft_consensus-itest.cc@208
PS11, Line 208: Status GetTabletCopyTargetSessionsCount(const ExternalDaemon& server,
> unimplemented?
Done
http://gerrit.cloudera.org:8080/#/c/8138/11/src/kudu/integration-tests/raft_consensus-nonvoter-itest.cc
File src/kudu/integration-tests/raft_consensus-nonvoter-itest.cc:
PS11:
> I think this file should be names raft_consensus_nonvoter-itest.cc
Done
http://gerrit.cloudera.org:8080/#/c/8138/11/src/kudu/integration-tests/raft_consensus-nonvoter-itest.cc@250
PS11, Line 250: AssertEventually
> use ASSERT_EVENTUALLY() here and below; see the macro definition for an exp
Done
http://gerrit.cloudera.org:8080/#/c/8138/11/src/kudu/integration-tests/raft_consensus-nonvoter-itest.cc@257
PS11, Line 257: ASSERT_OK(GetTabletCopyTargetSessionsCount(ed_new_replica, &num_sessions));
> why not put this and the above assertion inside the same ASSERT_EVENTUALLY?
Done
http://gerrit.cloudera.org:8080/#/c/8138/11/src/kudu/integration-tests/raft_consensus-nonvoter-itest.cc@515
PS11, Line 515: That's
> nit: That
Done
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 16 Oct 2017 17:59:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [consensus] introduce adding NON VOTER members
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8138 )
Change subject: WIP [consensus] introduce adding NON_VOTER members
......................................................................
Patch Set 2:
Curious how the client(s) will respond to seeing a non-voter. Does the master report non-voters to clients when they request tablet locations, and if so, do the clients know that they can use non-voters as readable replicas?
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 27 Sep 2017 14:59:05 +0000
Gerrit-HasComments: No
[kudu-CR] [consensus] adding/removing NON VOTER members
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8138 )
Change subject: [consensus] adding/removing NON_VOTER members
......................................................................
Patch Set 15: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 15
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 17 Oct 2017 04:35:14 +0000
Gerrit-HasComments: No
[kudu-CR] WIP [consensus] adding/removing NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8138
to look at the new patch set (#8).
Change subject: WIP [consensus] adding/removing NON_VOTER members
......................................................................
WIP [consensus] adding/removing NON_VOTER members
Added ability to add and remove NON_VOTER member replicas.
Updated the kudu CLI accordingly. Also, added new integration test:
* RaftConsensusITest.AddNonVoterReplica
* RaftConsensusITest.AddThenRemoveNonVoterReplica
* RaftConsensusITest.NonVoterReplicasDoNotVote
WIP: Perhaps, I should continue gathering feedback on this patch
and adding more tests.
Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
---
M src/kudu/consensus/leader_election-test.cc
M src/kudu/consensus/leader_election.cc
M src/kudu/consensus/leader_election.h
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/quorum_util.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
9 files changed, 557 insertions(+), 58 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/8138/8
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [consensus] adding/removing NON VOTER members
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8138 )
Change subject: [consensus] adding/removing NON_VOTER members
......................................................................
Patch Set 15: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 15
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 17 Oct 2017 06:15:31 +0000
Gerrit-HasComments: No
[kudu-CR] [consensus] adding/removing NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8138 )
Change subject: [consensus] adding/removing NON_VOTER members
......................................................................
Patch Set 15: Verified+1
I'll fix RaftConsensusElectionITest.TombstonedVoteAfterFailedTabletCopy flake separately.
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 15
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 17 Oct 2017 15:20:29 +0000
Gerrit-HasComments: No
[kudu-CR] [consensus] adding/removing NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8138
to look at the new patch set (#12).
Change subject: [consensus] adding/removing NON_VOTER members
......................................................................
[consensus] adding/removing NON_VOTER members
Added ability to add and remove NON_VOTER member replicas.
Updated the kudu CLI accordingly. Also, added new integration test:
* RaftConsensusITest.AddNonVoterReplica
* RaftConsensusITest.AddThenRemoveNonVoterReplica
* RaftConsensusITest.NonVoterReplicasDoNotVote
Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
---
M src/kudu/consensus/leader_election-test.cc
M src/kudu/consensus/leader_election.cc
M src/kudu/consensus/leader_election.h
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/quorum_util.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/raft_consensus-itest-base.cc
M src/kudu/integration-tests/raft_consensus-itest-base.h
M src/kudu/integration-tests/raft_consensus-itest.cc
A src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
13 files changed, 673 insertions(+), 83 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/8138/12
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 12
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP [consensus] adding/removing NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8138
to look at the new patch set (#6).
Change subject: WIP [consensus] adding/removing NON_VOTER members
......................................................................
WIP [consensus] adding/removing NON_VOTER members
Added ability to add and remove NON_VOTER member replicas.
Updated the kudu CLI accordingly. Also, added new integration test:
* RaftConsensusITest.AddNonVoterReplica
* RaftConsensusITest.AddThenRemoveNonVoterReplica
* RaftConsensusITest.NonVoterReplicasDoNotVote
WIP: Perhaps, I should continue gathering feedback on this patch
and adding more tests.
Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
---
M src/kudu/consensus/leader_election-test.cc
M src/kudu/consensus/leader_election.cc
M src/kudu/consensus/leader_election.h
M src/kudu/consensus/metadata.proto
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
9 files changed, 488 insertions(+), 58 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/8138/6
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP [consensus] introduce adding NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8138
to look at the new patch set (#2).
Change subject: WIP [consensus] introduce adding NON_VOTER members
......................................................................
WIP [consensus] introduce adding NON_VOTER members
Added ability to add NON_VOTER member replicas into Raft configuration.
Updated the kudu CLI accordingly. Also, added a new integration test:
RaftConsensusITest.AddNonVoterReplica.
WIP: I need to resolve the issue with mismatch reported by ksck,
most likely it will be in a separate changelist.
Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
---
M src/kudu/consensus/quorum_util.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
3 files changed, 136 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/8138/2
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] WIP [consensus] adding/removing NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8138 )
Change subject: WIP [consensus] adding/removing NON_VOTER members
......................................................................
Patch Set 2:
(2 comments)
Thank you for the review!
http://gerrit.cloudera.org:8080/#/c/8138/2/src/kudu/integration-tests/raft_consensus-itest.cc
File src/kudu/integration-tests/raft_consensus-itest.cc:
http://gerrit.cloudera.org:8080/#/c/8138/2/src/kudu/integration-tests/raft_consensus-itest.cc@416
PS2, Line 416: // Get number of source tablet copy sessions at the specified server.
> nit: leave one blank line above this, for consistency.
Done
http://gerrit.cloudera.org:8080/#/c/8138/2/src/kudu/integration-tests/raft_consensus-itest.cc@3242
PS2, Line 3242: TEST_F(RaftConsensusITest, AddNonVoterReplica) {
> I think this is a good addition, but that, ultimately we also need to add n
Yep, that's a good idea -- I'm working on it.
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 02 Oct 2017 18:35:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] [consensus] adding/removing NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8138
to look at the new patch set (#10).
Change subject: [consensus] adding/removing NON_VOTER members
......................................................................
[consensus] adding/removing NON_VOTER members
Added ability to add and remove NON_VOTER member replicas.
Updated the kudu CLI accordingly. Also, added new integration test:
* RaftConsensusITest.AddNonVoterReplica
* RaftConsensusITest.AddThenRemoveNonVoterReplica
* RaftConsensusITest.NonVoterReplicasDoNotVote
Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
---
M src/kudu/consensus/leader_election-test.cc
M src/kudu/consensus/leader_election.cc
M src/kudu/consensus/leader_election.h
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/quorum_util.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/raft_consensus-itest-base.cc
M src/kudu/integration-tests/raft_consensus-itest-base.h
M src/kudu/integration-tests/raft_consensus-itest.cc
A src/kudu/integration-tests/raft_consensus-nonvoter-itest.cc
13 files changed, 684 insertions(+), 83 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/8138/10
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 10
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [consensus] adding/removing NON VOTER members
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8138 )
Change subject: [consensus] adding/removing NON_VOTER members
......................................................................
Patch Set 11:
(7 comments)
LGTM, just minor feedback
http://gerrit.cloudera.org:8080/#/c/8138/8/src/kudu/consensus/leader_election.cc
File src/kudu/consensus/leader_election.cc:
http://gerrit.cloudera.org:8080/#/c/8138/8/src/kudu/consensus/leader_election.cc@166
PS8, Line 166: tries
tried
http://gerrit.cloudera.org:8080/#/c/8138/11/src/kudu/integration-tests/raft_consensus-itest.cc
File src/kudu/integration-tests/raft_consensus-itest.cc:
http://gerrit.cloudera.org:8080/#/c/8138/11/src/kudu/integration-tests/raft_consensus-itest.cc@204
PS11, Line 204: Status GetTabletCopySourceSessionsCount(const ExternalDaemon& server,
unimplemented
http://gerrit.cloudera.org:8080/#/c/8138/11/src/kudu/integration-tests/raft_consensus-itest.cc@208
PS11, Line 208: Status GetTabletCopyTargetSessionsCount(const ExternalDaemon& server,
unimplemented?
http://gerrit.cloudera.org:8080/#/c/8138/11/src/kudu/integration-tests/raft_consensus-nonvoter-itest.cc
File src/kudu/integration-tests/raft_consensus-nonvoter-itest.cc:
PS11:
I think this file should be names raft_consensus_nonvoter-itest.cc
http://gerrit.cloudera.org:8080/#/c/8138/11/src/kudu/integration-tests/raft_consensus-nonvoter-itest.cc@250
PS11, Line 250: AssertEventually
use ASSERT_EVENTUALLY() here and below; see the macro definition for an explanation why
http://gerrit.cloudera.org:8080/#/c/8138/11/src/kudu/integration-tests/raft_consensus-nonvoter-itest.cc@257
PS11, Line 257: ASSERT_OK(GetTabletCopyTargetSessionsCount(ed_new_replica, &num_sessions));
why not put this and the above assertion inside the same ASSERT_EVENTUALLY?
http://gerrit.cloudera.org:8080/#/c/8138/11/src/kudu/integration-tests/raft_consensus-nonvoter-itest.cc@515
PS11, Line 515: That's
nit: That
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 16 Oct 2017 05:13:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [consensus] adding/removing NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8138
to look at the new patch set (#4).
Change subject: WIP [consensus] adding/removing NON_VOTER members
......................................................................
WIP [consensus] adding/removing NON_VOTER members
Added ability to add and remove NON_VOTER member replicas.
Updated the kudu CLI accordingly. Also, added new integration test:
* RaftConsensusITest.AddNonVoterReplica
* RaftConsensusITest.AddThenRemoveNonVoterReplica
WIP: Perhaps, I should continue gathering feedback on this patch
and adding more tests.
Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
---
M src/kudu/consensus/leader_election-test.cc
M src/kudu/consensus/leader_election.cc
M src/kudu/consensus/leader_election.h
M src/kudu/consensus/metadata.proto
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
9 files changed, 430 insertions(+), 58 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/8138/4
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP [consensus] adding/removing NON VOTER members
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8138 )
Change subject: WIP [consensus] adding/removing NON_VOTER members
......................................................................
Patch Set 6:
(8 comments)
This is looking pretty good although I have some comments. I don't think we need to add client tool support to this patch before merging it, since it simply adds infrastructure and features that are not possible to activate without extraordinary effort.
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/leader_election-test.cc
File src/kudu/consensus/leader_election-test.cc:
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/leader_election-test.cc@164
PS6, Line 164: ASSERT_FALSE(voter_uuids_.empty());
Because of this change, we need to either add NO_FATALS() to all callsites of this function or make this a CHECK
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/leader_election.cc
File src/kudu/consensus/leader_election.cc:
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/leader_election.cc@167
PS6, Line 167: LOG_WITH_PREFIX(WARNING) << Substitute(
how about a CHECK?
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/quorum_util-test.cc
File src/kudu/consensus/quorum_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/quorum_util-test.cc@34
PS6, Line 34: SetPeerInfo
perhaps rename this to AddNewPeer()
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/quorum_util-test.cc@37
PS6, Line 37: boost::optional<RaftPeerPB::MemberType> type = boost::none
I think it's better to require a type instead of make it optional since there isn't much of a use case for the optional IMHO
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/quorum_util-test.cc@41
PS6, Line 41: !!
We don't need !! in this context because boost::optional<> implements operator bool() const
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/raft_consensus.cc@410
PS6, Line 410: DisableFailureDetector();
Hmm. I think it would be best to be more proactive about enabling / disabling the failure detector when there is a config change and not starting it in the first place if the current replica is a non-voter.
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/integration-tests/raft_consensus-itest.cc
File src/kudu/integration-tests/raft_consensus-itest.cc:
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/integration-tests/raft_consensus-itest.cc@3287
PS6, Line 3287: TEST_F(RaftConsensusITest, AddNonVoterReplica) {
If the purpose of the test is simply to add a non-voter and then see if we tablet copy, why is this test so long? We could create a cluster with 2 tablet servers, create a table with 1 tablet and 1 voter replica, add a non-voter to the config, and ensure that it ends up with a copy of the tablet. We don't even need to write data to the table.
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/integration-tests/raft_consensus-itest.cc@3408
PS6, Line 3408: TEST_F(RaftConsensusITest, AddThenRemoveNonVoterReplica) {
Similar comment as above
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 04 Oct 2017 17:07:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [consensus] adding/removing NON VOTER members
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8138 )
Change subject: WIP [consensus] adding/removing NON_VOTER members
......................................................................
Patch Set 6:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/leader_election-test.cc
File src/kudu/consensus/leader_election-test.cc:
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/leader_election-test.cc@164
PS6, Line 164: ASSERT_FALSE(voter_uuids_.empty());
> Because of this change, we need to either add NO_FATALS() to all callsites
Good point. Indeed, CHECK() would be best since this assertion is to protect from programmer's errors.
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/leader_election.cc
File src/kudu/consensus/leader_election.cc:
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/leader_election.cc@167
PS6, Line 167: LOG_WITH_PREFIX(WARNING) << Substitute(
> how about a CHECK?
I thought about that, but decided that would be too much: if due do some bug some NON_VOTER replica sends a request to others, all others replicas will crash.
I think it's better to add DCHECK() in at the point where RequestVotePB is being sent out -- I'll try to do that. That way only the rogue non-voter replica will crash in such condition, and only for DEBUG builds.
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/quorum_util-test.cc
File src/kudu/consensus/quorum_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/quorum_util-test.cc@34
PS6, Line 34: SetPeerInfo
> perhaps rename this to AddNewPeer()
Done
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/quorum_util-test.cc@37
PS6, Line 37: boost::optional<RaftPeerPB::MemberType> type = boost::none
> I think it's better to require a type instead of make it optional since the
I just wanted to use it for the case when member_type() is not set. OK, I'll just write a separate piece of code for that.
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/quorum_util-test.cc@41
PS6, Line 41: !!
> We don't need !! in this context because boost::optional<> implements opera
Last time I checked I didn't see the operation you mentioned. However, optional has the following operator:
bool operator!() const
Anyway, this is no longer necessary.
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/consensus/raft_consensus.cc@410
PS6, Line 410: DisableFailureDetector();
> Hmm. I think it would be best to be more proactive about enabling / disabli
I'm not sure about that.
This place seemed a better candidate since this is how it's done for NON_PARTICIPANT role (see below) and also this method is called from ConsensusServiceImpl::RunLeaderElection(). Also, this method seems to be the lowest level where election-related code is initiated, and handling non-voter case here looks the safest way of doing so, not missing anything else from upper level.
So, placing it here get the job done in a status-quo way and also allows not to duplicate any code in ConsensusServiceImpl::RunLeaderElection().
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/integration-tests/raft_consensus-itest.cc
File src/kudu/integration-tests/raft_consensus-itest.cc:
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/integration-tests/raft_consensus-itest.cc@3287
PS6, Line 3287: TEST_F(RaftConsensusITest, AddNonVoterReplica) {
> If the purpose of the test is simply to add a non-voter and then see if we
I tried to do that without writing data, but then --tablet_copy_download_file_inject_latency_ms=1000 flag didn't have proper effect for some reason (that latency is necessary to reliably spot tablet copy session start and complete).
And yes, this test does more than just ensuring the tablet copy sessions starts -- the idea is to make sure the tablet is available and writing data to it does not produce any errors.
http://gerrit.cloudera.org:8080/#/c/8138/6/src/kudu/integration-tests/raft_consensus-itest.cc@3408
PS6, Line 3408: TEST_F(RaftConsensusITest, AddThenRemoveNonVoterReplica) {
> Similar comment as above
Yep, the comment is outdated -- thanks for pointing at that. I'll update it.
--
To view, visit http://gerrit.cloudera.org:8080/8138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2662d45ad9bb6a4bf325d4202c2ee619ffad02b7
Gerrit-Change-Number: 8138
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 04 Oct 2017 23:19:21 +0000
Gerrit-HasComments: Yes