Alexey Serbin 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: (5 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, an Thank you for the clarification. Unfortunately, that's already released with Kudu 1.6, so I'm not updating this enum. However, for future enums I'll strive to use the recommended style. 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! Yep, I also think that having more specific error codes is better. Thank for mentioning this on the Slack channel. 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 jus Indeed -- that would be more generic and will work as described, good observation! 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 Indeed. http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/tserver/heartbeater.cc File src/kudu/tserver/heartbeater.cc: http://gerrit.cloudera.org:8080/#/c/9565/2/src/kudu/tserver/heartbeater.cc@435 PS2, Line 435: if (info->replacement_scheme() == > Make this info->replacement_schema() != EVICT_FIRST, in case we ever add an Yep, that would be more correct and cleaner. Done. -- 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 <aser...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Comment-Date: Fri, 09 Mar 2018 20:50:04 +0000 Gerrit-HasComments: Yes
