Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10336 )
Change subject: [rebalancing] Add a rebalancing algorithm ...................................................................... Patch Set 18: (11 comments) Looks good, I just have some small nits for feedback. http://gerrit.cloudera.org:8080/#/c/10336/18/src/kudu/tools/rebalance_algo-test.cc File src/kudu/tools/rebalance_algo-test.cc: http://gerrit.cloudera.org:8080/#/c/10336/18/src/kudu/tools/rebalance_algo-test.cc@67 PS18, Line 67: per nit: s/per/by/ add comment: // Number of replicas of this table on each server in the cluster http://gerrit.cloudera.org:8080/#/c/10336/18/src/kudu/tools/rebalance_algo-test.cc@78 PS18, Line 78: table_replicas.{front(),...,back()}.size I'm confused by this syntax, maybe say something like: for t in table_replicas: assert(num_replicas_by_server.size() == tserver_uuids.size()) http://gerrit.cloudera.org:8080/#/c/10336/18/src/kudu/tools/rebalance_algo-test.cc@550 PS18, Line 550: { "B", "0", "2" }, just wondering, in this case, why do we move B before moving A again? They have equal skew here at step 2. http://gerrit.cloudera.org:8080/#/c/10336/18/src/kudu/tools/rebalance_algo-test.cc@626 PS18, Line 626: nit: extra line http://gerrit.cloudera.org:8080/#/c/10336/18/src/kudu/tools/rebalance_algo.h File src/kudu/tools/rebalance_algo.h: http://gerrit.cloudera.org:8080/#/c/10336/18/src/kudu/tools/rebalance_algo.h@30 PS18, Line 30: nit: extra space http://gerrit.cloudera.org:8080/#/c/10336/18/src/kudu/tools/rebalance_algo.h@91 PS18, Line 91: size_t nit: prefer int per GSG, see https://google.github.io/styleguide/cppguide.html#Integer_Types http://gerrit.cloudera.org:8080/#/c/10336/18/src/kudu/tools/rebalance_algo.h@143 PS18, Line 143: // None of the output parameters may be NULL. nit: document how to interpret 'intersection' coming back empty http://gerrit.cloudera.org:8080/#/c/10336/18/src/kudu/tools/rebalance_algo.h@148 PS18, Line 148: int32_t* replica_count_table, : int32_t* replica_count_total, : std::vector<std::string>* server_uuids, : std::vector<std::string>* intersection); nit: instead of 4 out-params, can we have a single struct out-param instead? It would be easier to keep track of, I think. http://gerrit.cloudera.org:8080/#/c/10336/18/src/kudu/tools/rebalance_algo.cc File src/kudu/tools/rebalance_algo.cc: http://gerrit.cloudera.org:8080/#/c/10336/18/src/kudu/tools/rebalance_algo.cc@89 PS18, Line 89: return Status::NotFound("no count for replica", to); It looks like this could leave the map in an inconsistent state if only one was erased? Should we add an assertion for that? i.e. DCHECK_EQ(0, found_from ^ found_to) or something http://gerrit.cloudera.org:8080/#/c/10336/18/src/kudu/tools/rebalance_algo.cc@105 PS18, Line 105: size_t nit: int http://gerrit.cloudera.org:8080/#/c/10336/18/src/kudu/tools/rebalance_algo.cc@330 PS18, Line 330: stable_sort nit: just wondering, how is std::stable_sort available as stable_sort when there is no "using std::stable_sort" in this file? -- 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: 18 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Wed, 06 Jun 2018 23:16:51 +0000 Gerrit-HasComments: Yes
