Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10399 )
Change subject: WIP [tools] first draft of rebalancing in kudu CLI ...................................................................... Patch Set 7: (11 comments) http://gerrit.cloudera.org:8080/#/c/10399/7/src/kudu/tools/rebalance-test.cc File src/kudu/tools/rebalance-test.cc: http://gerrit.cloudera.org:8080/#/c/10399/7/src/kudu/tools/rebalance-test.cc@277 PS7, Line 277: ARRAYSIZE > nit: From reading the comment at kudu/gutil/macros.h:116, we should prefer Done http://gerrit.cloudera.org:8080/#/c/10399/7/src/kudu/tools/rebalance.h File src/kudu/tools/rebalance.h: http://gerrit.cloudera.org:8080/#/c/10399/7/src/kudu/tools/rebalance.h@38 PS7, Line 38: ReplicaMoveInfo > Can you relate this struct to TableReplicaMove in rebalance-algo.{cc, h}? I Done http://gerrit.cloudera.org:8080/#/c/10399/7/src/kudu/tools/rebalance.h@44 PS7, Line 44: MovesInProgress > Could you add a comment explaining what the string key is? Done http://gerrit.cloudera.org:8080/#/c/10399/7/src/kudu/tools/rebalance.h@54 PS7, Line 54: if : // non-empty, that's the only tables which will be rebalanced; > nit: I think this clause is redundant. Done http://gerrit.cloudera.org:8080/#/c/10399/7/src/kudu/tools/rebalance.h@56 PS7, Line 56: whole cluster > nit: Can you be extra clear and say this means that all tables will be bala Done http://gerrit.cloudera.org:8080/#/c/10399/7/src/kudu/tools/rebalance.h@63 PS7, Line 63: rebalancing > rebalancer Done http://gerrit.cloudera.org:8080/#/c/10399/7/src/kudu/tools/rebalance.h@67 PS7, Line 67: replica_move_infos > Can you comment on what the parameters are for? Done http://gerrit.cloudera.org:8080/#/c/10399/7/src/kudu/tools/rebalance.h@70 PS7, Line 70: std::ostream* > nit: While it goes against the GSG's rules, I think it's standard to use a Done http://gerrit.cloudera.org:8080/#/c/10399/7/src/kudu/tools/rebalance.cc File src/kudu/tools/rebalance.cc: http://gerrit.cloudera.org:8080/#/c/10399/7/src/kudu/tools/rebalance.cc@48 PS7, Line 48: rebalance_move_single_replicas > This is supposed to be a temporary flag, right? Right. I think it's worth to keep this around while there are Kudu clusters with older code where KUDU-2443 is not yet fixed. This flag would not appear unless KUDU-2443 existed. http://gerrit.cloudera.org:8080/#/c/10399/7/src/kudu/tools/tool_action_cluster.cc File src/kudu/tools/tool_action_cluster.cc: http://gerrit.cloudera.org:8080/#/c/10399/7/src/kudu/tools/tool_action_cluster.cc@145 PS7, Line 145: const vector<string> master_addresses = Split(master_addresses_str, ","); > nit: Swap this line and the one before it. Done http://gerrit.cloudera.org:8080/#/c/10399/7/src/kudu/tools/tool_action_tablet.cc File src/kudu/tools/tool_action_tablet.cc: http://gerrit.cloudera.org:8080/#/c/10399/7/src/kudu/tools/tool_action_tablet.cc@98 PS7, Line 98: MoveReplicaImpl > Since this is now shared by the rebalancer and the move_tablet tool, I thin The tool_action_common.cc does not contain any tablet-related code. I'm not sure we want to move it there. I think the fact that it used both in tablet and cluster actions does not necessarily means we want it in tool_action_common.cc. Maybe, we should separate it into a file which is common between action_tablet and action_cluster? -- To view, visit http://gerrit.cloudera.org:8080/10399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I269ea1dcb0b528ad9f03308bac6b8769e2141238 Gerrit-Change-Number: 10399 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Sat, 26 May 2018 07:27:58 +0000 Gerrit-HasComments: Yes
