Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10399 )
Change subject: [tools] rebalancer in the kudu CLI tool ...................................................................... Patch Set 17: (17 comments) http://gerrit.cloudera.org:8080/#/c/10399/15/src/kudu/tools/rebalance.h File src/kudu/tools/rebalance.h: PS15: > nit: consider naming this rebalancer.h since it has the Rebalancer class Done http://gerrit.cloudera.org:8080/#/c/10399/15/src/kudu/tools/rebalance.h@129 PS15, Line 129: > nit: using the term "step" here is confusing because it sounds like it's fo Done http://gerrit.cloudera.org:8080/#/c/10399/15/src/kudu/tools/rebalance.h@152 PS15, Line 152: > mutable ref violates style guide Done http://gerrit.cloudera.org:8080/#/c/10399/15/src/kudu/tools/rebalance.h@171 PS15, Line 171: > Needs doc, including the API contract Done http://gerrit.cloudera.org:8080/#/c/10399/15/src/kudu/tools/rebalance.h@181 PS15, Line 181: > This comment is very vague. Please add info about key -> value for the cont Done http://gerrit.cloudera.org:8080/#/c/10399/15/src/kudu/tools/rebalance.cc File src/kudu/tools/rebalance.cc: http://gerrit.cloudera.org:8080/#/c/10399/15/src/kudu/tools/rebalance.cc@53 PS15, Line 53: > true seems like a more desirable default in a 3-4-3 cluster. We discussed it offline. Yep, that will be a good idea for future releases, but for now it's safer to keep it 'false' by default because of KUDU-2443. Another option would be 'auto', where the effective value for the flag is determined based on the version of Kudu master/tservers (the idea is to enable this for versions not affected by KUDU-2443). http://gerrit.cloudera.org:8080/#/c/10399/15/src/kudu/tools/rebalance.cc@56 PS15, Line 56: > can we just make this a --verbose or -v argument to the kudu cluster rebala I think --verbose flag is too generic and also it's already registered (flags are in global namespace, right)? http://gerrit.cloudera.org:8080/#/c/10399/15/src/kudu/tools/rebalance.cc@238 PS15, Line 238: > A comment here would be nice to indicate when we want to clear scheduled mo Done http://gerrit.cloudera.org:8080/#/c/10399/15/src/kudu/tools/rebalance.cc@239 PS15, Line 239: > typo Done http://gerrit.cloudera.org:8080/#/c/10399/15/src/kudu/tools/rebalance.cc@250 PS15, Line 250: > nit: DCHECK_EQ("", FindNonUniqueTablet(replica_moves)) would print the offe I updated this part, not it works a bit different. http://gerrit.cloudera.org:8080/#/c/10399/15/src/kudu/tools/rebalance.cc@479 PS15, Line 479: > I think RunStep() might be better named GetNextMoveSet() or NextMoves() or It's a good idea. I named it GetNextMoves(). http://gerrit.cloudera.org:8080/#/c/10399/15/src/kudu/tools/rebalance.cc@479 PS15, Line 479: > why pass pending_moves into this function instead of just directly referenc That's because I was thinking about moving the helper structures into a separate sub-class, and the instance of that sub-class would be kept in the code of the Rebalancer::Run() function. http://gerrit.cloudera.org:8080/#/c/10399/15/src/kudu/tools/rebalance.cc@559 PS15, Line 559: > Can you clarify whether this means we prefer to move other replicas before Done http://gerrit.cloudera.org:8080/#/c/10399/15/src/kudu/tools/rebalance.cc@660 PS15, Line 660: : > I'm having trouble understanding why you are adding i to the set here. That's for easier addressing suggested moves by the algorithm, by involved tablet servers. It's used in the FindNextMove() method. I'll add more comments on that: it looks pretty obscure, yes. http://gerrit.cloudera.org:8080/#/c/10399/15/src/kudu/tools/tool_action_cluster.cc File src/kudu/tools/tool_action_cluster.cc: http://gerrit.cloudera.org:8080/#/c/10399/15/src/kudu/tools/tool_action_cluster.cc@212 PS15, Line 212: nfig move_ > nit: consider removing the rebalance_ prefix from these command-line flags, Done http://gerrit.cloudera.org:8080/#/c/10399/15/src/kudu/tools/tool_replica_util.h File src/kudu/tools/tool_replica_util.h: http://gerrit.cloudera.org:8080/#/c/10399/15/src/kudu/tools/tool_replica_util.h@56 PS15, Line 56: > nit: s/Make/Request/ Done http://gerrit.cloudera.org:8080/#/c/10399/15/src/kudu/tools/tool_replica_util.h@78 PS15, Line 78: // 'tablet_id'. Neither 'is_complete' nor 'completion_status' output parameter > nit: doc the contract for completion_status and Status return from this fun The 'is_complete' parameter specifies whether the operation is pending or completed. The 'completion_status' is to specify what kind of completion is that: a success or a failure. -- 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: 17 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Sat, 02 Jun 2018 04:56:21 +0000 Gerrit-HasComments: Yes
