Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14177 )
Change subject: KUDU-2780: create thread for auto-rebalancing ...................................................................... Patch Set 17: (7 comments) http://gerrit.cloudera.org:8080/#/c/14177/17/src/kudu/master/auto_rebalancer-test.cc File src/kudu/master/auto_rebalancer-test.cc: http://gerrit.cloudera.org:8080/#/c/14177/17/src/kudu/master/auto_rebalancer-test.cc@81 PS17, Line 81: & Having it written as it's now, it hints on the expectation of the number of tablet servers to change in background by some other actor while thing method is run. If so, then please add a comment about that. If not, then drop the reference:= I guess on 64-bit arch a reference is 64bit compared with 32 bit int if copying by value. http://gerrit.cloudera.org:8080/#/c/14177/17/src/kudu/master/auto_rebalancer-test.cc@97 PS17, Line 97: % nit: spacing http://gerrit.cloudera.org:8080/#/c/14177/17/src/kudu/master/auto_rebalancer-test.cc@359 PS17, Line 359: 1000 Given the exponential back-off behavior of ASSERT_EVENTUALLY under CheckSomeMovesScheduled() with cap of 1024ms, maybe make it at least 2000 ms to be sure we are about to sample the metrics at right time (i.e. when there is something still in progress)? With wider margins we will have better guarantees sampling the metrics at the right time. http://gerrit.cloudera.org:8080/#/c/14177/17/src/kudu/master/auto_rebalancer.h File src/kudu/master/auto_rebalancer.h: http://gerrit.cloudera.org:8080/#/c/14177/17/src/kudu/master/auto_rebalancer.h@178 PS17, Line 178: mutable Is 'mutable' is really needed here? http://gerrit.cloudera.org:8080/#/c/14177/17/src/kudu/master/auto_rebalancer.cc File src/kudu/master/auto_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/14177/17/src/kudu/master/auto_rebalancer.cc@707 PS17, Line 707: nit: remove the extra space http://gerrit.cloudera.org:8080/#/c/14177/17/src/kudu/master/auto_rebalancer.cc@748 PS17, Line 748: orig_leader_uuid Why is this change? http://gerrit.cloudera.org:8080/#/c/14177/17/src/kudu/rebalance/rebalancer.cc File src/kudu/rebalance/rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/14177/17/src/kudu/rebalance/rebalancer.cc@489 PS17, Line 489: std::mt19937 random_generator, : vector<string> tablet_ids, It seems Andrew's comment about copying/passing by values hasn't been addressed yet. -- 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: 17 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: Fri, 21 Feb 2020 20:01:14 +0000 Gerrit-HasComments: Yes
