Alexey Serbin 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 34: (11 comments) http://gerrit.cloudera.org:8080/#/c/18679/34//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18679/34//COMMIT_MSG@9 PS34, Line 9: kudu-master supports auto-rebalancer, but it must be decided whether : enable it before kudu master starting. kudu-master was able to run the auto-rebalancer, but toggling it on/off in runtime hadn't been supported. http://gerrit.cloudera.org:8080/#/c/18679/34//COMMIT_MSG@10 PS34, Line 10: This patch supports to turn : on/off `auto rebalancer` when kudu-master running by changing gflag : --auto_rebalancing_enabled. This patch adds new --auto_rebalancing_enabled flag which can be set in runtime via the SetFlag() RPC, toggling the auto-rebalancer on/off dynamically. http://gerrit.cloudera.org:8080/#/c/18679/34//COMMIT_MSG@14 PS34, Line 14: There are many independent, idle and isolated threads(which do : period tasks), they don't participant in thread pool. This patch start : to optimize them to use an unified thread pool to manager them. Could you please separate this into its own changelist? That would help in tracking changes and make reviewing easier. http://gerrit.cloudera.org:8080/#/c/18679/34/src/kudu/master/auto_rebalancer.h File src/kudu/master/auto_rebalancer.h: http://gerrit.cloudera.org:8080/#/c/18679/34/src/kudu/master/auto_rebalancer.h@81 PS34, Line 81: Schedule 'Run()' Task asynchronously. AFAIK, all the tasks are scheduled synchronously in ThreadPool: they either put into the queue right away or Schedule() returns corresponding error status at the call site, no? http://gerrit.cloudera.org:8080/#/c/18679/31/src/kudu/master/auto_rebalancer.cc File src/kudu/master/auto_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/18679/31/src/kudu/master/auto_rebalancer.cc@178 PS31, Line 178: scheduler_pool_token_ What if 'scheduler_pool_token_' isn't null at this point and already used to run tasks? http://gerrit.cloudera.org:8080/#/c/18679/34/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/18679/34/src/kudu/master/catalog_manager.h@1354 PS34, Line 1354: Auto rebalancer uses the thread pool to run periodic tasks. How about: Auto-rebalancer schedules periodic tasks on this thread pool. http://gerrit.cloudera.org:8080/#/c/18679/34/src/kudu/master/catalog_manager.h@1355 PS34, Line 1355: td::unique_ptr<ThreadPool> scheduler_pool_; Instead of introducing extra thread pool, is it possible to update the rebalancer tasks that it would wait with timeout on a condition variable that's checked and reset based on the value of the flag in its while() loop? http://gerrit.cloudera.org:8080/#/c/18679/31/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/18679/31/src/kudu/master/catalog_manager.cc@160 PS31, Line 160: DEFINE_int32(scheduler_pool_max_thread_num, 4, > We use thread_pool_token SERIAL mode, so it's not concurrent. Why not two? Two is 'little greater' than one as well. http://gerrit.cloudera.org:8080/#/c/18679/34/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/18679/34/src/kudu/master/catalog_manager.cc@157 PS34, Line 157: schedule_period_ms, scheduler_period_ms? http://gerrit.cloudera.org:8080/#/c/18679/34/src/kudu/master/catalog_manager.cc@160 PS34, Line 160: scheduler_pool_max_thread_num, 4 Why 4 and why there is a need to keep it configurable? Would default value of 2 suffice here? In other words, under what conditions it would be necessary to change this parameter? http://gerrit.cloudera.org:8080/#/c/18679/34/src/kudu/master/catalog_manager.cc@1001 PS34, Line 1001: .set_min_threads(1) : .set_max_threads(FLAGS_scheduler_pool_max_thread_num) : .set_enable_scheduler() : .set_schedule_period_ms(FLAGS_schedule_period_ms) : .Build(&scheduler_pool_)); The indent is messed up. See https://google.github.io/styleguide/cppguide.html for the C++ style guideline used in the Kudu project. -- 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: 34 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, 26 Aug 2022 02:24:00 +0000 Gerrit-HasComments: Yes
