Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/18679 )
Change subject: [master] KUDU-3389 support turning on/off auto rebalancer at runtime ...................................................................... Patch Set 32: (12 comments) I am very sorry for missing many crs. Thanks your time and crs. http://gerrit.cloudera.org:8080/#/c/18679/26/src/kudu/master/auto_rebalancer-test.cc File src/kudu/master/auto_rebalancer-test.cc: http://gerrit.cloudera.org:8080/#/c/18679/26/src/kudu/master/auto_rebalancer-test.cc@285 PS26, Line 285: // Make sure the auto-rebalancing can be toggled on > Did you miss this one? Sorry. Fix it. http://gerrit.cloudera.org:8080/#/c/18679/26/src/kudu/master/auto_rebalancer-test.cc@287 PS26, Line 287: cluster_opts_.num_masters = 1; : cluster_opts_.num_tablet_servers = 3; : ASSERT_OK(CreateAndStartCl > nit: these might be constexpr Done http://gerrit.cloudera.org:8080/#/c/18679/26/src/kudu/master/auto_rebalancer-test.cc@303 PS26, Line 303: } : int num_iterations = NumLoopIterations > What's the scheduler's period at this point? It's better to use the actual OK. done http://gerrit.cloudera.org:8080/#/c/18679/26/src/kudu/master/auto_rebalancer-test.cc@307 PS26, Line 307: : FLAGS_auto_rebalancing_enabled = true; : CheckSomeMovesScheduled(); : } > Wouldn't the ASSERT_EQ() be almost always true since it's executed immediat You are right, I fix it. http://gerrit.cloudera.org:8080/#/c/18679/26/src/kudu/master/auto_rebalancer.h File src/kudu/master/auto_rebalancer.h: http://gerrit.cloudera.org:8080/#/c/18679/26/src/kudu/master/auto_rebalancer.h@81 PS26, Line 81: Schedule 'Run()' Task asynchronously. > As far as I can see, the task itself is being _scheduled_ synchronously, no asynchronously. Tasks are submitted to the SchedulerThread's queue of threadpool, and then SchedulerThread submit the tasks to threadpool according to the exection MonoTime. http://gerrit.cloudera.org:8080/#/c/18679/26/src/kudu/master/auto_rebalancer.h@85 PS26, Line 85: // Leader master invokes 'DoReplicaRebalance()' when FLAGS_auto_rebalancing_enabled : // is set to 'true'. : void Run(); > How about: Done http://gerrit.cloudera.org:8080/#/c/18679/26/src/kudu/master/auto_rebalancer.h@88 PS26, Line 88: > Since this class is a task on its own and named AutoRebalancerTask already, Done http://gerrit.cloudera.org:8080/#/c/18679/26/src/kudu/master/auto_rebalancer.h@90 PS26, Line 90: tus DoReplicaRebalance(); > ? Fixed it to: // Move replicas if replicas is not balanced. http://gerrit.cloudera.org:8080/#/c/18679/26/src/kudu/master/auto_rebalancer.cc File src/kudu/master/auto_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/18679/26/src/kudu/master/auto_rebalancer.cc@194 PS26, Line 194: } > Did you miss this one? Sorry. I have missed most your crs. scheduler_pool_token_ is nullptr means it is shutdown, although it's work well, 'return Status::IllegalState()' is better. http://gerrit.cloudera.org:8080/#/c/18679/26/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/18679/26/src/kudu/master/catalog_manager.cc@158 PS26, Line 158: iod > I don't think it makes sense to have the default scheduling interval this s The parameter is for SchedulerThread in the threadpool, 100ms is reasonable. The SchedulerThread every schedule_period_ms check whether task need to run, so error of task execution time is less than 100ms. http://gerrit.cloudera.org:8080/#/c/18679/26/src/kudu/master/catalog_manager.cc@159 PS26, Line 159: > Every scheduler_period_ms the scheduler checks for tasks to run Done http://gerrit.cloudera.org:8080/#/c/18679/26/src/kudu/master/catalog_manager.cc@161 PS26, Line 161: cheduler thread pool"); > Maximum number of threads in the scheduler thread pool Done -- To view, visit http://gerrit.cloudera.org:8080/18679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifbcea6392b783843f89e019f1141dd8905366de5 Gerrit-Change-Number: 18679 Gerrit-PatchSet: 32 Gerrit-Owner: Yuqi Du <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Fri, 12 Aug 2022 06:54:30 +0000 Gerrit-HasComments: Yes
