Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14154 )
Change subject: KUDU-2914: Rebalance tool support moving replicas from some specific tablet servers ...................................................................... Patch Set 11: (12 comments) http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/rebalance/rebalance-test.cc File src/kudu/rebalance/rebalance-test.cc: http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/rebalance/rebalance-test.cc@192 PS10, Line 192: // The order of the key-value pairs whose keys compare equivalent is the order > I think it's worth documenting the essence of what this function uses as th Done http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/rebalance/rebalancer.h File src/kudu/rebalance/rebalancer.h: http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/rebalance/rebalancer.h@70 PS10, Line 70: If empty, run the rebalancing on : // all tablet servers in the cluster > Would it be beneficial to have the option to choose the former default beha Yes, if there are some unhealthy tservers in the cluster, and we didn't specify them in the ignored_tservers flag, the rebalancer tool would not run. http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool-test.cc File src/kudu/tools/rebalancer_tool-test.cc: http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool-test.cc@305 PS10, Line 305: FLAGS_num_tablet_servers = 5; : NO_FATALS(BuildAndStart()); : : // Assign one ignored tserver and move_replicas_from_ignored_tservers=true : // without setting it into maintenance mode. : { : > This is not necessary: newer Kudu clusters runs in 343 mode by default. Done http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool-test.cc@355 PS10, Line 355: << "stderr: " << err; : } > nit: join there two lines for better readability Done http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool-test.cc@360 PS10, Line 360: // 'output_replica_distribution_details' are both enabled. : auto* ts = cluster_->tablet > Add a comment explaining that this line show that the number of tablet repl Done http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool-test.cc@372 PS10, Line 372: "--move_replicas_from_ignored_tservers", > BTW, what happens if a tablet server, which is ignored and healthy in the b The rebalancer tool would ignore the unhealthy state of the tablet server since it has been specified 'ignored', it would just log some information about the unhealthy server and exit normally. I have added a test. http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool.cc File src/kudu/tools/rebalancer_tool.cc: http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool.cc@188 PS10, Line 188: fr > nit: from Done http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool.cc@631 PS10, Line 631: > Is this function only called when move_replicas_from_ignored_tservers=true? Yes, this function only called when move_replicas_from_ignored_tservers=true. http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool.cc@1294 PS10, Line 1294: ClusterInfo ci; : RETURN_NOT_OK(rebala > nit: shorten this to Done http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool.cc@1420 PS10, Line 1420: eplica > healthy Done http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool.cc@1421 PS10, Line 1421: // allow to run only when all healthy ignored tservers are in > nit: add a space before the parenthesis Done http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool.cc@1506 PS10, Line 1506: }); : > nit: would the following work here It would not work, ’ReplicaMove' was defined as a struct. -- To view, visit http://gerrit.cloudera.org:8080/14154 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I86cfb740030946c13db1a9ca63d241f4907d6c89 Gerrit-Change-Number: 14154 Gerrit-PatchSet: 11 Gerrit-Owner: Yifan Zhang <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hannah Nguyen <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Comment-Date: Mon, 28 Oct 2019 11:35:47 +0000 Gerrit-HasComments: Yes
