[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8644 )

Change subject: KUDU-1097 (patch 5a): Implement a bulk config change API
..

KUDU-1097 (patch 5a): Implement a bulk config change API

A "bulk" config change API is required to support simultaneously
modifying attributes on more than one peer in a config, such as when we
want to move a replica from one location to another.

The "traditional" config change API has been re-routed through the bulk
API so that we get some basic test coverage from the existing tests.

In addition to adding the bulk API, the following changes were made to
the MODIFY_PEER config change API which is currently unused:

 * The 'member_type' field is no longer required to be modified by a
   MODIFY_PEER config change operation, but *something* still must be
   modified to allow it to go through. This is enforced by checking the
   "before" and "after" RaftPeerPB instances with MessageDifferencer.
 * We now allow to modifying the leader replica's attributes (but still
   not its 'member_type' field).

A functional test was added to verify the new functionality of the bulk
change API.

Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
Reviewed-on: http://gerrit.cloudera.org:8080/8644
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin 
---
M src/kudu/consensus/consensus.proto
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
9 files changed, 538 insertions(+), 90 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Alexey Serbin: Looks good to me, approved

--
To view, visit http://gerrit.cloudera.org:8080/8644
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
Gerrit-Change-Number: 8644
Gerrit-PatchSet: 12
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8644 )

Change subject: KUDU-1097 (patch 5a): Implement a bulk config change API
..


Patch Set 11: Code-Review+2

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc@1728
PS9, Line 1728:  peer.permanent_
> I don't think we should try to handle that case in that way because it woul
yep, that sounds like a better solution.


http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc@1791
PS9, Line 1791: }
  : // A leader must be forced to step down before 
demoting it.
  :
> I'm not really worried about that. We'd have to add validation both here an
sgtm


http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc@1795
PS9, Line 1795:   Substitute("Cannot modify member type of peer 
$0 because it is the leader. "
> I think that would make it harder to support in the future, for example in
sgtm



--
To view, visit http://gerrit.cloudera.org:8080/8644
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
Gerrit-Change-Number: 8644
Gerrit-PatchSet: 11
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 29 Nov 2017 07:15:21 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8644 )

Change subject: KUDU-1097 (patch 5a): Implement a bulk config change API
..


Patch Set 11:

rev 10 addresses feedback from rev 9; rev 11 is just a rebase onto master


--
To view, visit http://gerrit.cloudera.org:8080/8644
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
Gerrit-Change-Number: 8644
Gerrit-PatchSet: 11
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 29 Nov 2017 05:00:04 +
Gerrit-HasComments: No

[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API

[Mike Percy (Code Review)]

Hello Tidy Bot, Alexey Serbin, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8644

to look at the new patch set (#10).

Change subject: KUDU-1097 (patch 5a): Implement a bulk config change API
..

KUDU-1097 (patch 5a): Implement a bulk config change API

A "bulk" config change API is required to support simultaneously
modifying attributes on more than one peer in a config, such as when we
want to move a replica from one location to another.

The "traditional" config change API has been re-routed through the bulk
API so that we get some basic test coverage from the existing tests.

In addition to adding the bulk API, the following changes were made to
the MODIFY_PEER config change API which is currently unused:

 * The 'member_type' field is no longer required to be modified by a
   MODIFY_PEER config change operation, but *something* still must be
   modified to allow it to go through. This is enforced by checking the
   "before" and "after" RaftPeerPB instances with MessageDifferencer.
 * We now allow to modifying the leader replica's attributes (but still
   not its 'member_type' field).

A functional test was added to verify the new functionality of the bulk
change API.

Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
---
M src/kudu/consensus/consensus.proto
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
9 files changed, 538 insertions(+), 90 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/8644/10
--
To view, visit http://gerrit.cloudera.org:8080/8644
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
Gerrit-Change-Number: 8644
Gerrit-PatchSet: 10
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8644 )

Change subject: KUDU-1097 (patch 5a): Implement a bulk config change API
..


Patch Set 9:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/8644/9//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8644/9//COMMIT_MSG@14
PS9, Line 14: api
> nit: API
Done


http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc@1704
PS9, Line 1704: for (const auto& item : req.config_changes()) {
> BTW, do we want to allow no-op configuration changes (i.e. requests with em
Nice catch. I don't think we should allow those, so I'll do a final check at 
the end to ensure we don't commit a no-op.


http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc@1719
PS9, Line 1719:   if (!peer.has_permanent_uuid()) {
> nit: PREDICT_FALSE() as well?
OK. We always had this, I just moved it.


http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc@1728
PS9, Line 1728: committed_config
> Perhaps, put new_config here instead?  That's to handle situations when the
I don't think we should try to handle that case in that way because it would 
make us sensitive to the ordering of the sub-requests which is too subtle to be 
useful, I think. I'll add handling for that in a different way by enforcing 
unique peer ids across all changes.


http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc@1731
PS9, Line 1731: committed_config
> ditto: new_config instead?
See above


http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc@1737
PS9, Line 1737:   if (peer.member_type() == RaftPeerPB::VOTER) {
  : num_voters_modified++;
  :   }
> nit: maybe, move this just before the 'break' for this case, as for the oth
Done, for this one only. It's awkward to do it for MODIFY_PEER and I don't 
think it's worth reorganizing the logic for.


http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc@1759
PS9, Line 1759: committed_config
> new_config?
See above


http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc@1791
PS9, Line 1791:   if (peer.attrs().has_promote()) {
  : 
modified_peer->mutable_attrs()->set_promote(peer.attrs().promote());
  :   }
> Do we want to prohibit adding the 'promote' attribute to voters?  At least,
I'm not really worried about that. We'd have to add validation both here and in 
ADD_PEER and it doesn't seem to me that VOTER + promote is dangerous or 
anything like that.


http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc@1795
PS9, Line 1795: 
modified_peer->mutable_attrs()->set_replace(peer.attrs().replace());
> Would it make sense to add DCHECK() to protect against adding 'replace' for
I think that would make it harder to support in the future, for example in a 
world where we had standby replicas, so my preference would be not to do that 
kind of enforcement.


http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/integration-tests/raft_config_change-itest.cc
File src/kudu/integration-tests/raft_config_change-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/integration-tests/raft_config_change-itest.cc@430
PS9, Line 430: }
> consider adding a scenario for the case when the config changes contain dup
Done



--
To view, visit http://gerrit.cloudera.org:8080/8644
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
Gerrit-Change-Number: 8644
Gerrit-PatchSet: 9
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 29 Nov 2017 04:56:01 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8644 )

Change subject: KUDU-1097 (patch 5a): Implement a bulk config change API
..


Patch Set 9:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/8644/9//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8644/9//COMMIT_MSG@14
PS9, Line 14: api
nit: API


http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc@1704
PS9, Line 1704: for (const auto& item : req.config_changes()) {
BTW, do we want to allow no-op configuration changes (i.e. requests with empty 
config_changes)?


http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc@1719
PS9, Line 1719:   if (!peer.has_permanent_uuid()) {
nit: PREDICT_FALSE() as well?

That's great: it seems like an improvement, since the previous version would 
just use the empty string in this case, popping an error at the later stage.


http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc@1728
PS9, Line 1728: committed_config
Perhaps, put new_config here instead?  That's to handle situations when the 
config_changes contains duplicate peers.


http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc@1731
PS9, Line 1731: committed_config
ditto: new_config instead?


http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc@1737
PS9, Line 1737:   if (peer.member_type() == RaftPeerPB::VOTER) {
  : num_voters_modified++;
  :   }
nit: maybe, move this just before the 'break' for this case, as for the other 
cases in this switch.


http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc@1759
PS9, Line 1759: committed_config
new_config?


http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc@1791
PS9, Line 1791:   if (peer.attrs().has_promote()) {
  : 
modified_peer->mutable_attrs()->set_promote(peer.attrs().promote());
  :   }
Do we want to prohibit adding the 'promote' attribute to voters?  At least, 
consider adding DCHECK() here in that case.


http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/consensus/raft_consensus.cc@1795
PS9, Line 1795: 
modified_peer->mutable_attrs()->set_replace(peer.attrs().replace());
Would it make sense to add DCHECK() to protect against adding 'replace' for 
non-voter replicas?


http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/integration-tests/raft_config_change-itest.cc
File src/kudu/integration-tests/raft_config_change-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8644/9/src/kudu/integration-tests/raft_config_change-itest.cc@430
PS9, Line 430: }
consider adding a scenario for the case when the config changes contain 
duplicate items.



--
To view, visit http://gerrit.cloudera.org:8080/8644
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
Gerrit-Change-Number: 8644
Gerrit-PatchSet: 9
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 28 Nov 2017 21:46:11 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API

[Mike Percy (Code Review)]

Hello Tidy Bot, Alexey Serbin, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8644

to look at the new patch set (#9).

Change subject: KUDU-1097 (patch 5a): Implement a bulk config change API
..

KUDU-1097 (patch 5a): Implement a bulk config change API

A "bulk" config change API is required to support simultaneously
modifying attributes on more than one peer in a config, such as when we
want to move a replica from one location to another.

The "traditional" config change API has been re-routed through the bulk
api so that we get some basic test coverage from the existing tests.

In addition to adding the bulk API, the following changes were made to
the MODIFY_PEER config change API which is currently unused:

 * The 'member_type' field is no longer required to be modified by a
   MODIFY_PEER config change operation, but *something* still must be
   modified to allow it to go through. This is enforced by checking the
   "before" and "after" RaftPeerPB instances with MessageDifferencer.
 * We now allow to modifying the leader replica's attributes (but still
   not its 'member_type' field).

A functional test was added to verify the new functionality of the bulk
change API.

Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
---
M src/kudu/consensus/consensus.proto
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
9 files changed, 500 insertions(+), 89 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/8644/9
--
To view, visit http://gerrit.cloudera.org:8080/8644
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
Gerrit-Change-Number: 8644
Gerrit-PatchSet: 9
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API

[Mike Percy (Code Review)]

Hello Tidy Bot, Alexey Serbin, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8644

to look at the new patch set (#8).

Change subject: KUDU-1097 (patch 5a): Implement a bulk config change API
..

KUDU-1097 (patch 5a): Implement a bulk config change API

A "bulk" config change API is required to support simultaneously
modifying attributes on more than one peer in a config, such as when we
want to move a replica from one location to another.

The "traditional" config change API has been re-routed through the bulk
api so that we get some basic test coverage from the existing tests.

In addition to adding the bulk API, the following changes were made to
the MODIFY_PEER config change API which is currently unused:

 * The 'member_type' field is no longer required to be modified by a
   MODIFY_PEER config change operation, but *something* still must be
   modified to allow it to go through.
 * It is now allowed to modify the leader replica's attributes (but
   still not its 'member_type').

A functional test was added to verify the new functionality of the bulk
change API.

Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
---
M src/kudu/consensus/consensus.proto
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
9 files changed, 506 insertions(+), 89 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/8644/8
--
To view, visit http://gerrit.cloudera.org:8080/8644
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
Gerrit-Change-Number: 8644
Gerrit-PatchSet: 8
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8644 )

Change subject: KUDU-1097 (patch 5a): Implement a bulk config change API
..


Patch Set 8:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/consensus/consensus.proto
File src/kudu/consensus/consensus.proto:

http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/consensus/consensus.proto@168
PS3, Line 168: required bytes tablet_id = 2
> Yes, I meant wire order for those fields.  That's just a nit, to keep it co
I don't see a reason to worry about that. The important thing is of course to 
avoid breaking pb wire compat, and secondarily it's nice to try to keep the 
source code field ordering consistent when possible. But the reason the 
dest_uuid is labeled with 4 in the other message is because it was added later, 
so that's just an artifact of proto evolution.


http://gerrit.cloudera.org:8080/#/c/8644/6/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
File src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8644/6/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@a650
PS6, Line 650:
 :
 :
 :
 :
 :
 :
 :
> If it does not contradict with the logic of how we track configuration chan
Well, it's not hard to keep the existing behavior. I can see that at least it's 
consistent with Add/Remove so I'll just restore the old behavior... if nothing 
is modified then it's an error.



--
To view, visit http://gerrit.cloudera.org:8080/8644
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
Gerrit-Change-Number: 8644
Gerrit-PatchSet: 8
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 28 Nov 2017 01:28:11 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8644 )

Change subject: KUDU-1097 (patch 5a): Implement a bulk config change API
..


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/consensus/consensus.proto
File src/kudu/consensus/consensus.proto:

http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/consensus/consensus.proto@168
PS3, Line 168: required bytes tablet_id = 2
> They are already in the order consistent with the other messages in this fi
Yes, I meant wire order for those fields.  That's just a nit, to keep it 
consistent with the rest of the messages in this file -- required fields come 
first there.  If it does not make much sense to you, feel free to ignore this.


http://gerrit.cloudera.org:8080/#/c/8644/6/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
File src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8644/6/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@a650
PS6, Line 650:
 :
 :
 :
 :
 :
 :
 :
> Do you think we should try to detect a no-op and reject it? I'm not sure it
If it does not contradict with the logic of how we track configuration changes, 
then it's OK, I think.  However, I see that for removing a peer which is not a 
member of config there would be an error, right?  And for an attempt to add a 
peer which is already in the config it will be an error as well, if I'm not 
mistaken.  Probably, that's fine.  At least, I don't see any contradictions 
here.

Anyway, it would be nice to have the new behavior covered by a test.  I'm not 
sure what is the best place for that, but it might be here (just invert the 
criteria for corresponding ASSERTs) or among the new tests you have recently 
added.


http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:

http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/tserver/tablet_service.cc@1044
PS3, Line 1044: ChangeConfigResponsePB
> I don't think so.
Thanks for confirming.



--
To view, visit http://gerrit.cloudera.org:8080/8644
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
Gerrit-Change-Number: 8644
Gerrit-PatchSet: 6
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 28 Nov 2017 00:00:08 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API

[Mike Percy (Code Review)]

Hello Tidy Bot, Alexey Serbin, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8644

to look at the new patch set (#7).

Change subject: KUDU-1097 (patch 5a): Implement a bulk config change API
..

KUDU-1097 (patch 5a): Implement a bulk config change API

A "bulk" config change API is required to support simultaneously
modifying attributes on more than one peer in a config, such as when we
want to move a replica from one location to another.

The "traditional" config change API has been re-routed through the bulk
api so that we get some basic test coverage from the existing tests.

In addition to adding the bulk API, the following changes were made to
the MODIFY_PEER config change API which is currently unused:

 * The 'member_type' field is no longer required to be modified by a
   MODIFY_PEER config change operation.
 * It is now allowed to modify the leader replica's attributes (but
   still not its 'member_type').

A functional test was added to verify the new functionality of the bulk
change API.

Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
---
M src/kudu/consensus/consensus.proto
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
9 files changed, 483 insertions(+), 112 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/8644/7
--
To view, visit http://gerrit.cloudera.org:8080/8644
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
Gerrit-Change-Number: 8644
Gerrit-PatchSet: 7
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8644 )

Change subject: KUDU-1097 (patch 5a): Implement a bulk config change API
..


Patch Set 6:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/consensus/consensus.proto
File src/kudu/consensus/consensus.proto:

http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/consensus/consensus.proto@168
PS3, Line 168: required bytes tablet_id = 2
> Mind setting this as the first field of the message?  That way, I think, it
They are already in the order consistent with the other messages in this file. 
Or are you talking about wire order? If so why does that matter?


http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/integration-tests/raft_config_change-itest.cc
File src/kudu/integration-tests/raft_config_change-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/integration-tests/raft_config_change-itest.cc@359
PS3, Line 359: 
> nit: does it make sense to check for the exact error type here?
will do


http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/integration-tests/raft_config_change-itest.cc@375
PS3, Line 375:   /*replace=*/ false, /*promote=*/ false } 
});
> mind adding a case to test adding/removing multiple voter replicas?
Good call, will do.


http://gerrit.cloudera.org:8080/#/c/8644/6/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
File src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8644/6/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@a650
PS6, Line 650:
 :
 :
 :
 :
 :
 :
 :
> Ah, that's probably because now change which does not change anything is ju
Do you think we should try to detect a no-op and reject it? I'm not sure it's 
very important to reject MODIFY_PEER no-ops, and the obvious way to detect it 
seems prone to human error in the future, so I just removed that behavior.


http://gerrit.cloudera.org:8080/#/c/8644/6/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@a665
PS6, Line 665:
 :
 :
 :
 :
 :
> ditto
same


http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:

http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/tserver/tablet_service.cc@1044
PS3, Line 1044: ChangeConfigResponsePB
> nit: just to clarify: we don't anticipate this to change in the context of
I don't think so.



--
To view, visit http://gerrit.cloudera.org:8080/8644
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
Gerrit-Change-Number: 8644
Gerrit-PatchSet: 6
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 27 Nov 2017 23:42:50 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8644 )

Change subject: KUDU-1097 (patch 5a): Implement a bulk config change API
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8644/6/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
File src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8644/6/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@a650
PS6, Line 650:
 :
 : 
 :
 :
 :
 :
 :
> I didn't see this covered in the new test TestBulkChangeConfig.  What is th
Ah, that's probably because now change which does not change anything is just a 
no-op.  Is it required by the new logic for bulk config change requests?



--
To view, visit http://gerrit.cloudera.org:8080/8644
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
Gerrit-Change-Number: 8644
Gerrit-PatchSet: 6
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 27 Nov 2017 23:38:28 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8644 )

Change subject: KUDU-1097 (patch 5a): Implement a bulk config change API
..


Patch Set 6:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/consensus/consensus.proto
File src/kudu/consensus/consensus.proto:

http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/consensus/consensus.proto@168
PS3, Line 168: required bytes tablet_id = 2
Mind setting this as the first field of the message?  That way, I think, it 
would be consistent with the rest of RequestPB messages at least in this file.


http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/integration-tests/raft_config_change-itest.cc
File src/kudu/integration-tests/raft_config_change-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/integration-tests/raft_config_change-itest.cc@359
PS3, Line 359:  
nit: does it make sense to check for the exact error type here?


http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/integration-tests/raft_config_change-itest.cc@375
PS3, Line 375:   /*replace=*/ false, /*promote=*/ false } 
});
mind adding a case to test adding/removing multiple voter replicas?


http://gerrit.cloudera.org:8080/#/c/8644/6/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
File src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8644/6/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@a650
PS6, Line 650:
 :
 :
 :
 :
 :
 :
 :
I didn't see this covered in the new test TestBulkChangeConfig.  What is the 
reason of removing this?


http://gerrit.cloudera.org:8080/#/c/8644/6/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@a665
PS6, Line 665:
 :
 :
 :
 :
 :
ditto


http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:

http://gerrit.cloudera.org:8080/#/c/8644/3/src/kudu/tserver/tablet_service.cc@1044
PS3, Line 1044: ChangeConfigResponsePB
nit: just to clarify: we don't anticipate this to change in the context of 
future changes in BulkChangeConfigRequest, right?  Right now I don't see any 
reason we would want to change it if evolving BulkChangeConfigRequestPB, but 
just in case.



--
To view, visit http://gerrit.cloudera.org:8080/8644
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
Gerrit-Change-Number: 8644
Gerrit-PatchSet: 6
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 27 Nov 2017 23:32:30 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API

[Mike Percy (Code Review)]

Hello Tidy Bot, Alexey Serbin, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8644

to look at the new patch set (#5).

Change subject: KUDU-1097 (patch 5a): Implement a bulk config change API
..

KUDU-1097 (patch 5a): Implement a bulk config change API

A "bulk" config change API is required to support simultaneously
modifying attributes on more than one peer in a config, such as when we
want to move a replica from one location to another.

The "traditional" config change API has been re-routed through the bulk
api so that we get some basic test coverage from the existing tests.

In addition to adding the bulk API, the following changes were made to
the MODIFY_PEER config change API which is currently unused:

 * The 'member_type' field is no longer required to be modified by a
   MODIFY_PEER config change operation.
 * It is now allowed to modify the leader replica's attributes (but
   still not its 'member_type').

A functional test was added to verify the new functionality of the bulk
change API.

Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
---
M src/kudu/consensus/consensus.proto
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
9 files changed, 445 insertions(+), 112 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/8644/5
--
To view, visit http://gerrit.cloudera.org:8080/8644
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
Gerrit-Change-Number: 8644
Gerrit-PatchSet: 5
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot