Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11662 )
Change subject: [rebalancer] location-aware rebalancer (part 2/3) ...................................................................... Patch Set 4: (15 comments) It looks pretty good to me but it's hard to thoroughly review this much code at once. I know things are still evolving a bit so I'll take a close look at a later changelist too. http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/rebalance_algo-test.cc File src/kudu/tools/rebalance_algo-test.cc: http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/rebalance_algo-test.cc@148 PS4, Line 148: balance.servers_by_total_replica_count.emplace( : count, tcc.tserver_uuids[tserver_idx]); Use EmplaceOrDie. http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/rebalance_algo-test.cc@282 PS4, Line 282: decltype(TestClusterConfig::servers_by_location){}, I think it'd be clearer to assign this quantity to a local variable like 'empty_location_assignment' and then pass the variable to each test case. http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/rebalance_algo.h File src/kudu/tools/rebalance_algo.h: http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/rebalance_algo.h@123 PS4, Line 123: , and http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/rebalance_algo.h@253 PS4, Line 253: LocationBalancingAlgo() = default; Is this needed? http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/rebalance_algo.h@264 PS4, Line 264: FindBestMove This could use a comment since it's hard to know what it does from the signature. http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/rebalance_algo.cc File src/kudu/tools/rebalance_algo.cc: http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/rebalance_algo.cc@192 PS4, Line 192: std::swap(*balance_info, info); Should we use move assignment here? http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/rebalance_algo.cc@407 PS4, Line 407: // Work on the most location-wise unbalanced tables first. This comment feels out of context. Maybe it needs to be expanded, or possible removed? http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/rebalance_algo.cc@414 PS4, Line 414: move some of it into the CluterBalanceInfo structure as is Even though it's your TODO one day someone else might implement it...could you flesh out what you mean by 'it'? http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/rebalance_algo.cc@430 PS4, Line 430: multimap<double How does this work if NaN is a key? That ought to never happen, so maybe we ought to CHECK for that when we insert. http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/rebalance_algo.cc@553 PS4, Line 553: avaiable available http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/rebalance_algo.cc@566 PS4, Line 566: withtin within http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/rebalance_algo.cc@570 PS4, Line 570: about replica which movement would : // violate the constraint of not having the majority of : // replicas at one location This part of the sentence kind of doesn't make sense. http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/rebalancer.h File src/kudu/tools/rebalancer.h: http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/rebalancer.h@160 PS4, Line 160: virtual ~Runner() = default; Is this needed? http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/rebalancer.h@209 PS4, Line 209: protected: Is there a defined order for public, protected, private? I would expected it to be public, then protected, then private. http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/tool_replica_util.cc File src/kudu/tools/tool_replica_util.cc: http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/tool_replica_util.cc@417 PS4, Line 417: if (cas_opid_idx) { : req.set_cas_config_opid_index(*cas_opid_idx); : } It'd be nice to combine this with L411. -- To view, visit http://gerrit.cloudera.org:8080/11662 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia4b0ccb3067a42f112e117b31f22d510f96e5234 Gerrit-Change-Number: 11662 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Thu, 18 Oct 2018 17:52:46 +0000 Gerrit-HasComments: Yes
