Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10612 )
Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements ...................................................................... Patch Set 8: (8 comments) 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: 'disabled'. The value of 'auto' means " : "turn it on/off depending on the replica m > This can be implied using the right flag tag (perhaps advanced or experimen Yep, it could be implied using the flag tags, but I'm not sure it's applicable in case of the flag for the 'kudu' utility. I think we can just drop this ominous sentence. http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@173 PS7, Line 173: client->default_admin_operation_timeout(), > Nit: add an inline arg to explain what this nullptr means. Done http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@180 PS7, Line 180: Version v; : if (!ParseVersion(version_str, &v).ok()) { > Nit: I think this would be simpler as a single DCHECK on "disabled", on L18 Done http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@220 PS7, Line 220: summaries }) { : for (const auto& summary : summaries) { : if (summary.version) { : if (!VersionSupportsRF1Movement(*summary.version)) { > I'll be honest; I don't understand this explanation. Could you clarify furt Indeed: the comment does not make much sense. Updated. http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@224 PS7, Line 224: LOG(INFO) << "found Kudu server of version '" << *summary.version > Nit: what's the point of this extra scope? Done http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@226 PS7, Line 226: > Don't we have to test this too? If not, why bother setting it to non-null? Indeed -- it should return false if it's 3-2-3. It seems like a typo. 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: > I don't think this belongs here. This is util code, so one would expect the Done 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: > Nit: drop Done -- 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: 8 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[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: Sat, 23 Jun 2018 01:58:52 +0000 Gerrit-HasComments: Yes
