Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18454 )
Change subject: [master] KUDU-3061 support auto rebalance tablet leaders across TServers ...................................................................... Patch Set 13: (20 comments) http://gerrit.cloudera.org:8080/#/c/18454/13/src/kudu/master/auto_leader_rebalancer.cc File src/kudu/master/auto_leader_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/18454/13/src/kudu/master/auto_leader_rebalancer.cc@92 PS13, Line 92: DCHECK(!thread_) << "AutoleaderRebalancerTask is already initialized"; thread_ is not initialized after calling Init() when FLAGS_auto_leader_rebalancing_enabled is false, it's a bit of strange that an object can be 'init' multiple times. http://gerrit.cloudera.org:8080/#/c/18454/13/src/kudu/master/auto_leader_rebalancer.cc@93 PS13, Line 93: RETURN_NOT_OK(MessengerBuilder("auto-leader-rebalancer").Build(&messenger_)); Is it needed to initialize messenger_ when FLAGS_auto_leader_rebalancing_enabled is false? http://gerrit.cloudera.org:8080/#/c/18454/13/src/kudu/master/auto_leader_rebalancer.cc@116 PS13, Line 116: Status AutoLeaderRebalancerTask::LeaderRebalance(const scoped_refptr<TableInfo>& table_info, How about rename to RunLeaderRebalanceForTable, or something like that? http://gerrit.cloudera.org:8080/#/c/18454/13/src/kudu/master/auto_leader_rebalancer.cc@117 PS13, Line 117: std:: nit: same http://gerrit.cloudera.org:8080/#/c/18454/13/src/kudu/master/auto_leader_rebalancer.cc@117 PS13, Line 117: std:: nit: can be omitted http://gerrit.cloudera.org:8080/#/c/18454/13/src/kudu/master/auto_leader_rebalancer.cc@136 PS13, Line 136: kudu:: nit: can be omitted http://gerrit.cloudera.org:8080/#/c/18454/13/src/kudu/master/auto_leader_rebalancer.cc@165 PS13, Line 165: if (ContainsKey(uuid_leaders_map, uuid)) { : uuid_leaders_map[uuid].emplace_back(tablet->id()); : } else { : uuid_leaders_map.insert({uuid, vector<string>({tablet->id()})}); : } can be simplified by: auto& uuid_leaders = LookupOrInsert(&uuid_leaders_map, uuid, {}); uuid_leaders.emplace_back(tablet->id()); http://gerrit.cloudera.org:8080/#/c/18454/13/src/kudu/master/auto_leader_rebalancer.cc@174 PS13, Line 174: if (ContainsKey(follower_uuid_map, tablet->id())) { : follower_uuid_map[tablet->id()].emplace_back(uuid); : } else { : follower_uuid_map.insert({tablet->id(), vector<string>({uuid})}); : } also can be simplified as above. http://gerrit.cloudera.org:8080/#/c/18454/13/src/kudu/master/auto_leader_rebalancer.cc@184 PS13, Line 184: auto it = uuid_replicas_map.find(ts_info.permanent_uuid()); : if (ContainsKey(uuid_replicas_map, ts_info.permanent_uuid())) { : it->second.emplace_back(tablet->id()); : } else { : uuid_replicas_map.insert( : {ts_info.permanent_uuid(), std::vector<string>({tablet->id()})}); : } same http://gerrit.cloudera.org:8080/#/c/18454/13/src/kudu/master/auto_leader_rebalancer.cc@203 PS13, Line 203: auto it1 = uuid_leaders_map.find(uuid); : if (it1 != uuid_leaders_map.end()) { : leader_count = it1->second.size(); : } else { : leader_count = 0; : } FindWithDefault() in map-util.h may help. http://gerrit.cloudera.org:8080/#/c/18454/13/src/kudu/master/auto_leader_rebalancer.cc@209 PS13, Line 209: auto it2 = uuid_replicas_map.find(uuid); : if (it2 != uuid_replicas_map.end()) { : replica_count = it2->second.size(); : } else { : // no replica, skip it : continue; : } use FindOrNull? http://gerrit.cloudera.org:8080/#/c/18454/13/src/kudu/master/auto_leader_rebalancer.cc@218 PS13, Line 218: if (mode == AutoLeaderRebalancerTask::ExecuteMode::TEST) { I think use VLOG here wouble be more reasonable than depends on mode. http://gerrit.cloudera.org:8080/#/c/18454/13/src/kudu/master/auto_leader_rebalancer.cc@234 PS13, Line 234: std:: nit: can be omitted. http://gerrit.cloudera.org:8080/#/c/18454/13/src/kudu/master/auto_leader_rebalancer.cc@247 PS13, Line 247: std:: omit http://gerrit.cloudera.org:8080/#/c/18454/13/src/kudu/master/auto_leader_rebalancer.cc@359 PS13, Line 359: number_of_loop_iterations_for_test_ How about add a metric for it? http://gerrit.cloudera.org:8080/#/c/18454/13/src/kudu/master/auto_leader_rebalancer.cc@366 PS13, Line 366: set<string> tserver_uuid_set; why use std::set? aren't the tserver uuids unique? http://gerrit.cloudera.org:8080/#/c/18454/13/src/kudu/master/auto_leader_rebalancer.cc@390 PS13, Line 390: , Substitute("RunLeaderRebalancer not ok") this log is helpless http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_rebalancer.cc File src/kudu/master/auto_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_rebalancer.cc@174 PS12, Line 174: if (FLAGS_auto_rebalancing_enabled) { > I want to support to execute RunReplicaRebalancer() by external tool cmd la You can do it in the next patch, it not needed to modify it now, and you can enable it in unit test manually. http://gerrit.cloudera.org:8080/#/c/18454/13/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/18454/13/src/kudu/master/catalog_manager.h@771 PS13, Line 771: > *table nit: >* table http://gerrit.cloudera.org:8080/#/c/18454/13/src/kudu/master/catalog_manager.h@776 PS13, Line 776: > *table nit: >* table -- To view, visit http://gerrit.cloudera.org:8080/18454 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibfb60d8759a93b6a19238637c27df4f6b1cac918 Gerrit-Change-Number: 18454 Gerrit-PatchSet: 13 Gerrit-Owner: Yuqi Du <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[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: Sun, 19 Jun 2022 09:04:11 +0000 Gerrit-HasComments: Yes
