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 12: (16 comments) 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: "FLAGS_auto_leader_rebalancing_interval_seconds"); nit: need improve description http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_leader_rebalancer.cc@136 PS12, Line 136: std:: nit: can be omit http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_leader_rebalancer.cc@147 PS12, Line 147: // GetTabletLocations() will fail if the catalog manager is not the Better to comment closer to the code. http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_leader_rebalancer.cc@165 PS12, Line 165: auto it = uuid_leaders_map.find(uuid); : if (it != uuid_leaders_map.end()) { : it->second.emplace_back(tablet->id()); : } else { : uuid_leaders_map.insert({uuid, std::vector<string>({tablet->id()})}); : } There maybe some useful function you can use, see src/kudu/gutil/map-util.h http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_leader_rebalancer.cc@175 PS12, Line 175: auto it = follower_uuid_map.find(tablet->id()); : if (it != follower_uuid_map.end()) { : follower_uuid_map[tablet->id()].push_back(uuid); : } else { : follower_uuid_map.insert({tablet->id(), std::vector<string>({uuid})}); : } Same http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_leader_rebalancer.cc@186 PS12, Line 186: auto it = uuid_replicas_map.find(ts_info.permanent_uuid()); : if (it == uuid_replicas_map.end()) { : uuid_replicas_map.insert( : {ts_info.permanent_uuid(), std::vector<string>({tablet->id()})}); : } else { : it->second.emplace_back(tablet->id()); : } Same http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_leader_rebalancer.cc@229 PS12, Line 229: LOG(INFO) << uuid I think this log would be in VLOG(x) level. http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_leader_rebalancer.cc@323 PS12, Line 323: auto it = leader_uuid_host_port_map.find(leader_uuid); There are several code can be optimized by util functions in src/kudu/gutil/map-util.h http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_leader_rebalancer.cc@337 PS12, Line 337: LOG(INFO) << strings::Substitute( Also should be in VLOG(x) http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_leader_rebalancer.cc@368 PS12, Line 368: std::set<std::string> set<string> http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_leader_rebalancer.cc@392 PS12, Line 392: Status s = RunLeaderRebalancer(); You can use WARN_NOT_OK to simplify code. 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 false? http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_rebalancer.cc@180 PS12, Line 180: ; nit: remove it http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_rebalancer.cc@264 PS12, Line 264: bool should_stop = false; Is it necessary to judge whether should stop loop by an extra variable 'should_stop'? The loop is using shutdown_.WaitFor, it will break automatically when shutdown_ become 0. http://gerrit.cloudera.org:8080/#/c/18454/12/src/kudu/master/auto_rebalancer.cc@267 PS12, Line 267: // stop the loop nit: remove this useless comment 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 -- 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: 12 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: Wed, 01 Jun 2022 10:09:10 +0000 Gerrit-HasComments: Yes
