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 19: (9 comments) http://gerrit.cloudera.org:8080/#/c/18454/19/src/kudu/client/flex_partitioning_client-test.cc File src/kudu/client/flex_partitioning_client-test.cc: http://gerrit.cloudera.org:8080/#/c/18454/19/src/kudu/client/flex_partitioning_client-test.cc@341 PS19, Line 341: // Using one-level hash bucketing { 3, "key" } as the custom hash schema Sorry, I didn't get the point. Is this related to this patch? http://gerrit.cloudera.org:8080/#/c/18454/19/src/kudu/master/auto_leader_rebalancer.cc File src/kudu/master/auto_leader_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/18454/19/src/kudu/master/auto_leader_rebalancer.cc@68 PS19, Line 68: 10, nit: would it be more convenient if the gflag name and default value are in one line when we grep it? http://gerrit.cloudera.org:8080/#/c/18454/19/src/kudu/master/auto_leader_rebalancer.cc@72 PS19, Line 72: 3600, same http://gerrit.cloudera.org:8080/#/c/18454/19/src/kudu/master/auto_leader_rebalancer.cc@96 PS19, Line 96: "catalog-manager" nit: better to use the same category name, i.e. "catalog manager", as other threads in CatalogManager, then they will be in the same group on webUI. http://gerrit.cloudera.org:8080/#/c/18454/19/src/kudu/master/auto_leader_rebalancer.cc@103 PS19, Line 103: if (thread_) { Is there any case thread_ is not initialized? http://gerrit.cloudera.org:8080/#/c/18454/19/src/kudu/master/auto_leader_rebalancer.cc@181 PS19, Line 181: const vector<string>& leaders = FindWithDefault(uuid_leaders_map, uuid, {}); : int32_t leader_count = leaders.size(); nit: move these code closer to the place where it used, means after L188. http://gerrit.cloudera.org:8080/#/c/18454/19/src/kudu/master/auto_leader_rebalancer.cc@192 PS19, Line 192: int32_t should_transfer_count = leader_count - (replica_count / table_data.num_replicas() + 1); Could you please add some comments to describe the algorithm? http://gerrit.cloudera.org:8080/#/c/18454/19/src/kudu/master/auto_rebalancer.cc File src/kudu/master/auto_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/18454/19/src/kudu/master/auto_rebalancer.cc@a181 PS19, Line 181: why remove it? http://gerrit.cloudera.org:8080/#/c/18454/19/src/kudu/master/auto_rebalancer.cc@177 PS19, Line 177: [this]() { this->RunLoop(); }, &thread_); : nit: alignment and remove blank line. -- 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: 19 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: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Tue, 05 Jul 2022 16:44:39 +0000 Gerrit-HasComments: Yes
