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 <mpe...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 28 Nov 2017 21:46:11 +0000 Gerrit-HasComments: Yes
