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 34: (10 comments) Thanks your time and crs. I'll split the patch into 2 crs. 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 Done 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 run Done 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 OK. 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 What you said after :, Yes. I think this is called asynchronous execution. What you said "all the tasks are scheduled synchronously in ThreadPool:" , I don't know the accurate expression. Schedule() push task to SchedulerThread' queue, and by timepoint push the task to threadpool queue and then threadpool run the task. 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 t AutoRebalanceTask is only one instance and run Init only once, so scheduler_pool_token_ must be null. I can add a DCHECK at this line. 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: Done 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 reba Yes, that's ok. But my plan is not only for this. It start some works and prepare something. The direct purpose, the threadpool can unified some seperator threads, eg: master's 'auto-rebalancer' (the Thread), 'expired-reserved-tables-deleter', 'hms-notification-log-listener' ("catalog manager", "bgtasks"), and later's leader-rebalancer's Thread, and some other threads. As tserver has some others thread More details: https://issues.apache.org/jira/browse/KUDU-3364 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? Done 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 Done 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. I don't think it's messed up. I have not found some rules forbidden this style at Google C++ Style. In fact I run .clang-format there is no warnings for this. This is results auto reformat. 'set_min_threads' ... are methods, use 4 indents to distinct class 'ThreadPoolBuilder' object is normal. As below's ThreadPoolBuilder styles before to try unified styles. other files I don't change it. The command: ' git show -U0 | build-support/clang_format_diff.sh -i -p1 ', also format the codes as this. -- 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 12:27:46 +0000 Gerrit-HasComments: Yes
