[kudu-CR] WIP [consensus] introduce adding NON VOTER members

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 27 Sep 2017 17:45:47 +
Gerrit-HasComments: No

[kudu-CR] WIP [consensus] introduce adding NON VOTER members

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 27 Sep 2017 14:59:05 +
Gerrit-HasComments: No

[kudu-CR] WIP [consensus] introduce adding NON VOTER members

[David Ribeiro Alves (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 26 Sep 2017 20:56:36 +
Gerrit-HasComments: Yes

[kudu-CR] WIP [consensus] introduce adding NON VOTER members

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] WIP [consensus] introduce adding NON VOTER members

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 25 Sep 2017 23:55:50 +
Gerrit-HasComments: Yes

[kudu-CR] WIP [consensus] introduce adding NON VOTER members

[David Ribeiro Alves (Code Review)]

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 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 25 Sep 2017 23:02:31 +
Gerrit-HasComments: Yes

[kudu-CR] WIP [consensus] introduce adding NON VOTER members

[David Ribeiro Alves (Code Review)]

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 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 25 Sep 2017 23:01:35 +
Gerrit-HasComments: Yes

[kudu-CR] WIP [consensus] introduce adding NON VOTER members

[Alexey Serbin (Code Review)]

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