Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10336 )
Change subject: [rebalancing] Add a rebalancing algorithm ...................................................................... Patch Set 14: (11 comments) Just some nits on comments and formatting. Sorry I'm kind of picky about wording. I think this is about good to go. We should get another set of eyes on it, though. http://gerrit.cloudera.org:8080/#/c/10336/14/src/kudu/tools/CMakeLists.txt File src/kudu/tools/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/10336/14/src/kudu/tools/CMakeLists.txt@153 PS14, Line 153: kudu_tools_util : kudu_tools_test_util : kudu_tools_rebalance : itest_util : mini_cluster nit: Not sure how much we care about alphabetically sorting here, but this is all out of order. http://gerrit.cloudera.org:8080/#/c/10336/14/src/kudu/tools/rebalance_algo-test.cc File src/kudu/tools/rebalance_algo-test.cc: http://gerrit.cloudera.org:8080/#/c/10336/14/src/kudu/tools/rebalance_algo-test.cc@78 PS14, Line 78: table_replicas.{front(),...,back()}.size ultra-nit: You mean table_replicas.{front(),...,back()}.num_replicas_per_server.size(). That's pretty wordy, so maybe it'd be easier to say, "For each t in table_replicas, t.num_replicas_per_server.size() == tserver_uuids.size()." http://gerrit.cloudera.org:8080/#/c/10336/14/src/kudu/tools/rebalance_algo-test.cc@117 PS14, Line 117: tserver_idx < tcc.tserver_uuids.size() nit: It's a little easier to read if each statement in the for clause is on its own line (1 + 1 + 1) instead of 2 + 1. http://gerrit.cloudera.org:8080/#/c/10336/14/src/kudu/tools/rebalance_algo-test.cc@119 PS14, Line 119: at on http://gerrit.cloudera.org:8080/#/c/10336/14/src/kudu/tools/rebalance_algo-test.cc@152 PS14, Line 152: PICK_FIRST nit: Mention why this option is being used. http://gerrit.cloudera.org:8080/#/c/10336/14/src/kudu/tools/rebalance_algo-test.cc@372 PS14, Line 372: configuration configurations http://gerrit.cloudera.org:8080/#/c/10336/14/src/kudu/tools/rebalance_algo-test.cc@372 PS14, Line 372: to have just one one-move that need one move http://gerrit.cloudera.org:8080/#/c/10336/14/src/kudu/tools/rebalance_algo-test.cc@376 PS14, Line 376: that's why multiples of virtually same that's why we test multiple virtually identical http://gerrit.cloudera.org:8080/#/c/10336/14/src/kudu/tools/rebalance_algo-test.cc@437 PS14, Line 437: a http://gerrit.cloudera.org:8080/#/c/10336/14/src/kudu/tools/rebalance_algo-test.cc@491 PS14, Line 491: a http://gerrit.cloudera.org:8080/#/c/10336/14/src/kudu/tools/rebalance_algo-test.cc@597 PS14, Line 597: Unbalanced (both table- and cluster-wise) and simple enough configurations to : // make them balanced moving many replicas around. This is a little hard to read. Maybe "Simple configurations that are both table- and cluster-wise unbalanced and require many replica moves to make them balanced." -- To view, visit http://gerrit.cloudera.org:8080/10336 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5a8050ee79117a2ae2f6f88740ed25656946cfb4 Gerrit-Change-Number: 10336 Gerrit-PatchSet: 14 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: Wed, 30 May 2018 06:57:59 +0000 Gerrit-HasComments: Yes
