Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/18679 )
Change subject: [master] support turning on/off auto rebalancer at runtime ...................................................................... Patch Set 26: (13 comments) > Patch Set 24: Code-Review+1 > > (3 comments) > > looks good to me except some nits. Thanks a lot. http://gerrit.cloudera.org:8080/#/c/18679/24//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18679/24//COMMIT_MSG@7 PS24, Line 7: turn > turning Done http://gerrit.cloudera.org:8080/#/c/18679/24//COMMIT_MSG@9 PS24, Line 9: supports auto > supports Done http://gerrit.cloudera.org:8080/#/c/18679/24//COMMIT_MSG@18 PS24, Line 18: ttempt. > refactoring? Done http://gerrit.cloudera.org:8080/#/c/18679/24/src/kudu/master/auto_rebalancer-test.cc File src/kudu/master/auto_rebalancer-test.cc: http://gerrit.cloudera.org:8080/#/c/18679/24/src/kudu/master/auto_rebalancer-test.cc@303 PS24, Line 303: // Wait a schedule period. > there is a sleep in ASSERT_EVENTUALLY, it's not needed to sleep manually. The intent is checking 'NumLoopIterations' is not increment when FLAGS_auto_rebalancing_enabled=false. If no sleep, any conditions can passed at this. But the writes can be better. DONE http://gerrit.cloudera.org:8080/#/c/18679/24/src/kudu/master/auto_rebalancer-test.cc@312 PS24, Line 312: FLAGS_auto_rebalancing_enabled = true; > can we use CheckNoMovesScheduled and/or CheckSomeMovesScheduled instead? Yes. Check Replica moves is better. DONE http://gerrit.cloudera.org:8080/#/c/18679/24/src/kudu/master/auto_rebalancer.h File src/kudu/master/auto_rebalancer.h: http://gerrit.cloudera.org:8080/#/c/18679/24/src/kudu/master/auto_rebalancer.h@81 PS24, Line 81: // Schedule `Rebalance()` Task asynchronously. > Add a blurb explaining an essence of what this method does. Done http://gerrit.cloudera.org:8080/#/c/18679/24/src/kudu/master/auto_rebalancer.h@86 PS24, Line 86: // true, invoke `DoReplicaRebalance()` to move replicas if > Add a blurb explaining the essence of what this method does. Done http://gerrit.cloudera.org:8080/#/c/18679/24/src/kudu/master/auto_rebalancer.cc File src/kudu/master/auto_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/18679/24/src/kudu/master/auto_rebalancer.cc@175 PS24, Line 175: scheduler pool is not initialized > This is supposed to be a message that describes a problem -- it prints out assertion fails means 'catalog_manager_->scheduler_pool()' is nullptr, so 'scheduler pool is not initialized' is right? http://gerrit.cloudera.org:8080/#/c/18679/24/src/kudu/master/auto_rebalancer.cc@179 PS24, Line 179: return S > Why CHECK_OK() instead of RETURN_NOT_OK()? Fixed. http://gerrit.cloudera.org:8080/#/c/18679/24/src/kudu/master/auto_rebalancer.cc@188 PS24, Line 188: : Status AutoRebalancerTask::ScheduleRebalance() { : i > nit: I meant we can use scheduler_pool_token_.reset(); directly, it's safe Fixed http://gerrit.cloudera.org:8080/#/c/18679/24/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/18679/24/src/kudu/master/catalog_manager.h@1254 PS24, Line 1254: iodic tasks. > execution of a delayed task Done http://gerrit.cloudera.org:8080/#/c/18679/24/src/kudu/master/catalog_manager.h@1254 PS24, Line 1254: use the thr > Please outline what this is actually used for, not what it can be used for. Done http://gerrit.cloudera.org:8080/#/c/18679/24/src/kudu/master/catalog_manager.h@1255 PS24, Line 1255: ::unique_ptr<ThreadPoo > run periodic tasks 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: 26 Gerrit-Owner: Yuqi Du <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Thu, 28 Jul 2022 06:42:27 +0000 Gerrit-HasComments: Yes
