Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10336 )
Change subject: [rebalancing] add a rebalancing algorithm ...................................................................... Patch Set 18: (11 comments) Thank you for the review! 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/ Done 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_repli Done 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 I think that's just an implementation detail, where the order among the candidate moves with the same skew is ordered by table identifier. In practice, we strive to choose among such candidates randomly: notice that here in tests we use EqualSkewOption::PICK_FIRST, but by default it EqualSkewOption::PICK_RANDOM. These test scenarios use PICK_FIRST because they compare the result with the reference. http://gerrit.cloudera.org:8080/#/c/10336/18/src/kudu/tools/rebalance_algo-test.cc@626 PS18, Line 626: > nit: extra line Done 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 Done 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.h Done 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 Done 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 After some consideration, I decided to leave this as is for now. 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 Good catch -- I added some code to restore the consistency of the container is such cases, and I added DCHECK for that as well. http://gerrit.cloudera.org:8080/#/c/10336/18/src/kudu/tools/rebalance_algo.cc@105 PS18, Line 105: size_t > nit: int Done 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 Ah, that's a good question. From the other side, we don't need std::stable_sort here, just std::sort() is enough since we don't expect UUIDs to be the same, and we don't need to preserve the order of same UUIDs anyway. -- 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: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Thu, 07 Jun 2018 02:19:39 +0000 Gerrit-HasComments: Yes
