Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/8901 )
Change subject: [master/tserver] enforce re-replication scheme consistency ...................................................................... Patch Set 4: (9 comments) http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/consensus/replica_management.proto File src/kudu/consensus/replica_management.proto: http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/consensus/replica_management.proto@23 PS4, Line 23: // Message to communicate on the replica management details between involved : // actors. "Communicates replica management information between servers." http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/consensus/replica_management.proto@39 PS4, Line 39: required I think this field (like pretty much all proto fields) should be optional, since it will be painful to drop or deprecate this field if we change how the replacement scheme is encoded in the future. See https://stackoverflow.com/a/31814967. http://gerrit.cloudera.org:8080/#/c/8901/4/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/8901/4/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1685 PS4, Line 1685: it in http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1686 PS4, Line 1686: make makes http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1714 PS4, Line 1714: // The easiest way to have everything setup is to start the cluster with : // compatible parameters. Could you move this comment above where the flags are set? I was confused by the flags until I read down to here. http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/master/master.proto@80 PS4, Line 80: INCOMPATIBILITY Consider "Incompatible". http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/master/master.proto@293 PS4, Line 293: iff nit: extra f? Given the identical phrasing below I'm guessing this f is extra and you don't mean iff as in "if and only if". http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/master/master.proto@294 PS4, Line 294: optional consensus.ReplicaManagementInfoPB replica_management_info = 7; Would it be better to put some kind of repeated feature flag here instead of a field per specific compatibility? I feel like, if we ever reach 3+ possible configurations we want to report here, we'll wish it was organized differently. http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/master/master_service.cc@158 PS4, Line 158: what is nit: remove -- To view, visit http://gerrit.cloudera.org:8080/8901 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I71c4c2e72bb2d62cec6de0f6d00b418377e8ae85 Gerrit-Change-Number: 8901 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Wed, 27 Dec 2017 15:53:57 +0000 Gerrit-HasComments: Yes
