Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9565 )
Change subject: [master] add feature flag for 3-4-3 scheme ...................................................................... Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/consensus/replica_management.proto File src/kudu/consensus/replica_management.proto: http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/consensus/replica_management.proto@27 PS2, Line 27: UNKNOWN = 999; The recommended style for new enums is to have the UNKNOWN variant be 0, and count up from there. It's recommended because that's the only way enums can be defined in proto3, and if we ever want to move to proto3 it will make the transition easier. We have many cases of UNKNOWN = 999 in the codebase because we later went back and added UNKNOWN after we already had variants assigned to 0. I recommend changing this enum, but only if we haven't already released a version containing it, since the change is backwards incompatible. http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/master/master.proto@80 PS2, Line 80: INCOMPATIBLE_REPLICA_MANAGEMENT = 12; Big +1 on this, thanks! http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/master/master.proto@715 PS2, Line 715: PREPARE_REPLACEMENT_BEFORE_EVICTION = 4; Consider making this feature flag REPLICA_MANAGEMENT, since it's really just a requirement that the master recognize and deal with the replica_management_info field. If in the future a new replica replacement policy type is added, we don't actually need a new feature flag. In that scenario we'd add the policy type to the ReplacementScheme enum, the tserver would send the value to the master, and the master would recognize that it doesn't understand the policy type because it would deserialize it as UNKNOWN. http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/master/master_service.cc@164 PS2, Line 164: if (scheme != ts_scheme) { Circling back to my comment in master.proto, this code is written correctly to handle UNKNOWN. In that case we fall into this block and return INCOMPATIBLE_REPLICA_MANAGEMENT, which is good. -- To view, visit http://gerrit.cloudera.org:8080/9565 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0e34a6fffc375720b26c2641e88057975d9190b9 Gerrit-Change-Number: 9565 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Comment-Date: Fri, 09 Mar 2018 17:53:04 +0000 Gerrit-HasComments: Yes
