Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10612 )
Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements ...................................................................... Patch Set 7: (8 comments) Can you articulate how the existing tests provide sufficient coverage for --move_single_replicas=auto? http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc File src/kudu/tools/tool_action_cluster.cc: http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@101 PS7, Line 101: It's best to keep the default value for this " : "flag unless you know what you are doing." This can be implied using the right flag tag (perhaps advanced or experimental). http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@173 PS7, Line 173: nullptr, is_343_scheme); Nit: add an inline arg to explain what this nullptr means. http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@180 PS7, Line 180: DCHECK(boost::iequals(FLAGS_move_single_replicas, "enabled") || : boost::iequals(FLAGS_move_single_replicas, "disabled")); Nit: I think this would be simpler as a single DCHECK on "disabled", on L185. http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@220 PS7, Line 220: That's because write : // operations in progress (if any) cannot be committed until the destination : // replica (added as a voter) is caught up. Catching up the destination : // voter replica might take much longer than write operation's timeout. I'll be honest; I don't understand this explanation. Could you clarify further? http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@224 PS7, Line 224: { Nit: what's the point of this extra scope? http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@226 PS7, Line 226: is_343_scheme Don't we have to test this too? If not, why bother setting it to non-null? http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/util/version_util.h File src/kudu/util/version_util.h: http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/util/version_util.h@60 PS7, Line 60: bool VersionSupportsRF1Movement(const std::string& version_str); I don't think this belongs here. This is util code, so one would expect the functions and classes to be Kudu-agnostic. http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/util/version_util.cc File src/kudu/util/version_util.cc: http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/util/version_util.cc@85 PS7, Line 85: std:: Nit: drop -- To view, visit http://gerrit.cloudera.org:8080/10612 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e Gerrit-Change-Number: 10612 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[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: Thu, 21 Jun 2018 23:48:11 +0000 Gerrit-HasComments: Yes
