Yuqi Du 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: (13 comments) Thanks your crs. http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_leader_rebalancer.cc File src/kudu/master/auto_leader_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_leader_rebalancer.cc@71 PS12, Line 71: DEFINE_uint32(auto_leader_rebalancing_interval_seconds, 3600, > nit: need improve description Done http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_leader_rebalancer.cc@136 PS12, Line 136: map<s > nit: can be omit Done http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_leader_rebalancer.cc@147 PS12, Line 147: CatalogManager::TSInfosDict ts_infos_dict; > Better to comment closer to the code. Remote it. Done http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_leader_rebalancer.cc@165 PS12, 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()})}); : } : l > There maybe some useful function you can use, see src/kudu/gutil/map-util.h Changed it, and I'll study it later. http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_leader_rebalancer.cc@229 PS12, Line 229: } > I think this log would be in VLOG(x) level. Done http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_leader_rebalancer.cc@337 PS12, Line 337: table_data.name(), task.first, leader_uuid, task.second.second); > Also should be in VLOG(x) Done http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_leader_rebalancer.cc@368 PS12, Line 368: if (e->PresumedDead > set<string> Done http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_leader_rebalancer.cc@392 PS12, Line 392: } > You can use WARN_NOT_OK to simplify code. Done 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) { > Why still create AutoRebalancerTask when FLAGS_auto_rebalancing_enabled is I want to support to execute RunReplicaRebalancer() by external tool cmd laster when FLAGS_auto_rebalancing_enabled is false. And now, it was used by rebalancer unit test. http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_rebalancer.cc@180 PS12, Line 180: > nit: remove it Done http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_rebalancer.cc@264 PS12, Line 264: > Is it necessary to judge whether should stop loop by an extra variable 'sho You are right. Fixed. http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_rebalancer.cc@267 PS12, Line 267: const ClusterLocalityInfo& locality, > nit: remove this useless comment Done http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/catalog_manager.cc@3695 PS12, Line 3695: scoped_refptr<TableInfo> *table) { > nit: alignment Done -- 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: Thu, 16 Jun 2022 09:50:00 +0000 Gerrit-HasComments: Yes
