Hannah Nguyen has posted comments on this change. ( http://gerrit.cloudera.org:8080/14177 )
Change subject: KUDU-2780: create thread for auto-rebalancing ...................................................................... Patch Set 14: (18 comments) Still need to look into implementing a condition variable waiting vs. sleeping http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/master/auto_rebalancer-test.cc File src/kudu/master/auto_rebalancer-test.cc: http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/master/auto_rebalancer-test.cc@159 PS14, Line 159: shared_ptr > nit: any special requirements to necessitate usage of std::shared_ptr? If Done http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/master/auto_rebalancer-test.cc@161 PS14, Line 161: shared_ptr > ditto Done http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/master/auto_rebalancer-test.cc@329 PS14, Line 329: if (!AllowSlowTests()) { : LOG(WARNING) << "test is skipped; set KUDU_ALLOW_SLOW_TESTS=1 to run"; : return; : } > nit: consider using SKIP_IF_SLOW_NOT_ALLOWED macro instead Done http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/master/auto_rebalancer-test.cc@345 PS14, Line 345: SetupWorkLoad(kNumTablets, /*num_replicas*/3); > I suspect this fails if number of tablet servers in cluster is less than 3, Done http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/master/auto_rebalancer-test.cc@369 PS14, Line 369: for (auto replica : replicas) { > warning: loop variable is copied but only used as const reference; consider Done http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/master/auto_rebalancer.cc File src/kudu/master/auto_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/master/auto_rebalancer.cc@209 PS14, Line 209: if (paused_) continue; > Is this set anywhere? If not, remove it? Done http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/master/auto_rebalancer.cc@273 PS14, Line 273: all_moves_complete > Do we need this? Could we just check if replica_moves is empty? Done http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/master/auto_rebalancer.cc@660 PS14, Line 660: *all_moves_complete = true; > nit: it's generally best practice to not update out-params if the method fa Actually just removed this param; can just check size of replica_moves per your comment above, thanks :) http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/master/auto_rebalancer.cc@663 PS14, Line 663: stack<int> moves_to_remove; > Is the LIFO order here important? And is it important to actually pop every Done http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/master/auto_rebalancer.cc@670 PS14, Line 670: move_completion_status > Not used? Is that intentional? Done http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/master/ts_manager.cc File src/kudu/master/ts_manager.cc: http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/master/ts_manager.cc@214 PS14, Line 214: for (const TSDescriptorMap::value_type& entry : servers_by_id_) { > nit: in C++11 you can use auto for this, e.g. Done http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/rebalance/rebalancer.h File src/kudu/rebalance/rebalancer.h: http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/rebalance/rebalancer.h@171 PS14, Line 171: The source and destination replicas are determined by the elements of the : // 'tablet_ids' container and tablet server UUIDs TableReplicaMove::from and : // TableReplica::to correspondingly. > nit: not from this patch, but this makes it seem like 'tablet_ids' is an in Done http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/rebalance/rebalancer.h@260 PS14, Line 260: ' > nit: remove Done http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/rebalance/rebalancer.h@261 PS14, Line 261: in move > nit: it's easy to misread this as "replicas in 'move'". Maybe "replicas in Done http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/rebalance/rebalancer.h@264 PS14, Line 264: Status SelectReplicasToMove( > nit: maybe also describe the behavior re: potentially returning an empty st Done http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/rebalance/rebalancer.cc File src/kudu/rebalance/rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/rebalance/rebalancer.cc@487 PS14, Line 487: const TableReplicaMove& move, : const unordered_map<string, TabletExtraInfo>& extra_info_by_tablet_id, : std::mt19937 random_generator, : vector<string> tablet_ids, : unordered_set<string>* tablets_in_move, : vector<Rebalancer::ReplicaMove>* replica_moves) { > nit: indent by four spaces Done http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/rebalance/rebalancer.cc@497 PS14, Line 497: string > nit: make this a const ref? Can't declare a reference unless I initialize it, which has to wait until the for loop below http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/tools/rebalancer_tool.cc File src/kudu/tools/rebalancer_tool.cc: http://gerrit.cloudera.org:8080/#/c/14177/14/src/kudu/tools/rebalancer_tool.cc@981 PS14, Line 981: > nit: extra spaces Done -- To view, visit http://gerrit.cloudera.org:8080/14177 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifca25d1063c07047cf2123e6792b3c7395be20e4 Gerrit-Change-Number: 14177 Gerrit-PatchSet: 14 Gerrit-Owner: Hannah Nguyen <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hannah Nguyen <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 18 Feb 2020 18:01:23 +0000 Gerrit-HasComments: Yes
