[kudu-CR] [master/tserver] enforce re-replication scheme consistency

["Alexey Serbin (Code Review)" <ge...@cloudera.org>]

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

Change subject: [master/tserver] enforce re-replication scheme consistency
......................................................................


Patch Set 4:

(10 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."
Done


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, 
It's a good call.


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
Done


http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1686
PS4, Line 1686: make
> makes
Done


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 b
Done


http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1739
PS4, Line 1739:   SleepFor(MonoDelta::FromMilliseconds(heartbeat_interval_ms * 3));
> warning: either cast from 'int' to 'int64_t' (aka 'long') is ineffective, o
Done


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".
I think the codes in this enum mean 'the reason for an error', so from that perspective 'INCOMPATIBILITY' looks better.


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 ext
Done


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 o
I thought about the feature flag, but I decided to go with a field.

The feature flag is to indicate whether some feature is supported/understood by the implementation, not whether it's turned on or off.  So, it should be more than just one bit for that anyway.

The other reason I opted for a field is that I think we should eventually get rid of different replica management schemes.

However, I will give it a second thought.


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
Done



-- 
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 <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 04 Jan 2018 00:44:49 +0000
Gerrit-HasComments: Yes