Alexey Serbin 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) Thanks a lot for the review! I'm working on splitting this patch into few. 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. IIRC, that's std::multimap, so it cannot be anything but success when inserting any key, even a duplicate one. 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 'e That sounds like a good idea. BTW, that ugliness appeared because of old libstdc++ on CentOS 6 and alike, otherwise that's just an empty map would suffice (i.e. {}). 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 Done http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/rebalance_algo.h@253 PS4, Line 253: LocationBalancingAlgo() = default; > Is this needed? Nope, it's not. 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 sign Done 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? Done 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 possib Done 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 Done. 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 Done http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/rebalance_algo.cc@553 PS4, Line 553: avaiable > available Done http://gerrit.cloudera.org:8080/#/c/11662/4/src/kudu/tools/rebalance_algo.cc@566 PS4, Line 566: withtin > within Done 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. Done 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? Having virtual destructor in the base class is a must when objects are passed by references/pointers to the base class, otherwise some surprises might happen because correct destructor of the derived class wont be called. Basically, destructors in that sense behave the same as regular virtual functions. 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 i Yes, that's the order dictated by the style guide, IIUC. And in this case there is public and protected, but no private section. 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. Woops. Good catch! -- 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: Fri, 19 Oct 2018 14:24:24 +0000 Gerrit-HasComments: Yes
