You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2017/11/27 07:32:57 UTC
[kudu-CR] WIP: KUDU-1097 (patch 5): Implement a bulk config change API
Hello Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/8644
to review the following change.
Change subject: WIP: KUDU-1097 (patch 5): Implement a bulk config change API
......................................................................
WIP: KUDU-1097 (patch 5): 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.
This WIP is not "wired up" yet.
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/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
5 files changed, 133 insertions(+), 57 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/8644/1
--
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: newchange
Gerrit-Change-Id: I928a1622d48049c9ad3223b76549a2822bccc30c
Gerrit-Change-Number: 8644
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 27 Nov 2017 23:32:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 29 Nov 2017 05:00:04 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] WIP: KUDU-1097 (patch 5): Implement a bulk config change API
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8644 )
Change subject: WIP: KUDU-1097 (patch 5): Implement a bulk config change API
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8644/1/src/kudu/consensus/consensus.proto
File src/kudu/consensus/consensus.proto:
http://gerrit.cloudera.org:8080/#/c/8644/1/src/kudu/consensus/consensus.proto@123
PS1, Line 123: MODIFY_PEER = 3;
> no, see the previous patch; now MODIFY_PEER can change attributes or really
Ah, sure -- I look at them in the wrong order.
http://gerrit.cloudera.org:8080/#/c/8644/1/src/kudu/consensus/consensus.proto@165
PS1, Line 165: // UUID of server this request is addressed to.
: optional bytes dest_uuid = 1;
:
: required bytes tablet_id = 2;
> This is consistent with the rest of the RPCs though
I meant the field ordering for the PB, i.e. put tablet_id = 1 and dest_uuid = 2. At least ChangeConfigRequestPB and VoteRequestPB the required fields are put first.
--
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: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 27 Nov 2017 08:31:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 28 Nov 2017 21:46:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: KUDU-1097 (patch 5): Implement a bulk config change API
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 (#2).
Change subject: WIP: KUDU-1097 (patch 5): Implement a bulk config change API
......................................................................
WIP: KUDU-1097 (patch 5): 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').
TODO: fix some outstanding test issues
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/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
8 files changed, 342 insertions(+), 84 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/8644/2
--
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: 2
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] WIP: KUDU-1097 (patch 5): Implement a bulk config change API
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8644 )
Change subject: WIP: KUDU-1097 (patch 5): Implement a bulk config change API
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8644/1/src/kudu/consensus/consensus.proto
File src/kudu/consensus/consensus.proto:
http://gerrit.cloudera.org:8080/#/c/8644/1/src/kudu/consensus/consensus.proto@123
PS1, Line 123: MODIFY_PEER = 3;
And we will need CHANGE_ATTRS or something like that, right?
http://gerrit.cloudera.org:8080/#/c/8644/1/src/kudu/consensus/consensus.proto@165
PS1, Line 165: // UUID of server this request is addressed to.
: optional bytes dest_uuid = 1;
:
: required bytes tablet_id = 2;
nit: maybe, put the required field first?
http://gerrit.cloudera.org:8080/#/c/8644/1/src/kudu/tserver/tablet_service.h
File src/kudu/tserver/tablet_service.h:
PS1:
Maybe, it's worth making those style changes in a separate changelist?
--
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: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 27 Nov 2017 07:47:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 27 Nov 2017 23:42:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097 (patch 5): Implement a bulk config change API
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 (#3).
Change subject: KUDU-1097 (patch 5): Implement a bulk config change API
......................................................................
KUDU-1097 (patch 5): 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/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
8 files changed, 401 insertions(+), 84 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/8644/3
--
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: 3
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1097 (patch 5): Implement a bulk config change API
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 (#4).
Change subject: KUDU-1097 (patch 5): Implement a bulk config change API
......................................................................
KUDU-1097 (patch 5): 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, 446 insertions(+), 112 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/8644/4
--
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: 4
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 29 Nov 2017 04:56:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 29 Nov 2017 07:15:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 27 Nov 2017 23:38:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
---
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 28 Nov 2017 00:00:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] WIP: KUDU-1097 (patch 5): Implement a bulk config change API
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8644 )
Change subject: WIP: KUDU-1097 (patch 5): Implement a bulk config change API
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8644/1/src/kudu/consensus/consensus.proto
File src/kudu/consensus/consensus.proto:
http://gerrit.cloudera.org:8080/#/c/8644/1/src/kudu/consensus/consensus.proto@123
PS1, Line 123: MODIFY_PEER = 3;
> And we will need CHANGE_ATTRS or something like that, right?
no, see the previous patch; now MODIFY_PEER can change attributes or really change anything
http://gerrit.cloudera.org:8080/#/c/8644/1/src/kudu/consensus/consensus.proto@165
PS1, Line 165: // UUID of server this request is addressed to.
: optional bytes dest_uuid = 1;
:
: required bytes tablet_id = 2;
> nit: maybe, put the required field first?
This is consistent with the rest of the RPCs though
http://gerrit.cloudera.org:8080/#/c/8644/1/src/kudu/tserver/tablet_service.h
File src/kudu/tserver/tablet_service.h:
PS1:
> Maybe, it's worth making those style changes in a separate changelist?
OK
--
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: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 27 Nov 2017 07:50:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 28 Nov 2017 01:28:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097 (patch 5a): Implement a bulk config change API
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot