Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13539 )
Change subject: [rebalance] Add '--ignored_tservers' flag to rebalancer ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/13539/1/src/kudu/tools/rebalancer.h File src/kudu/tools/rebalancer.h: http://gerrit.cloudera.org:8080/#/c/13539/1/src/kudu/tools/rebalancer.h@68 PS1, Line 68: // NOLINTNEXTLINE(google-explicit-constructor) > If the goal is to allow implicit conversions here, you can silence the warn Done http://gerrit.cloudera.org:8080/#/c/13539/1/src/kudu/tools/rebalancer.h@68 PS1, Line 68: // NOLINTNEXTLINE(google-explicit-constructor) > If the goal is to allow implicit conversions here, you can silence the warn Done http://gerrit.cloudera.org:8080/#/c/13539/1/src/kudu/tools/rebalancer.h@83 PS1, Line 83: // rebalancing only when all tablet servers in cluster are healthy. > Trailing whitespace here. Done http://gerrit.cloudera.org:8080/#/c/13539/1/src/kudu/tools/rebalancer.h@239 PS1, Line 239: et of ignor > Given the pattern of usage of this container, it might be better if it were Done http://gerrit.cloudera.org:8080/#/c/13539/1/src/kudu/tools/rebalancer.cc File src/kudu/tools/rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/13539/1/src/kudu/tools/rebalancer.cc@1231 PS1, Line 1231: // automatic re-replication or get unexpected errors while moving replicas. : for (const auto& s : raw_info.tserver_summaries) { : if (s.health != KsckServerHealth::HEALTHY) { : if (ContainsKey(ignored_tservers_, s.uuid)) { : continue; : } > Agreed with Alexey on using std::unordered_set to speed up this lookup. Use Done http://gerrit.cloudera.org:8080/#/c/13539/1/src/kudu/tools/rebalancer.cc@1445 PS1, Line 1445: std::move(ignored_tservers), > Some of the lines here are too long; could you rewrap? Done http://gerrit.cloudera.org:8080/#/c/13539/1/src/kudu/tools/rebalancer_tool-test.cc File src/kudu/tools/rebalancer_tool-test.cc: http://gerrit.cloudera.org:8080/#/c/13539/1/src/kudu/tools/rebalancer_tool-test.cc@221 PS1, Line 221: specifying > specifying Done -- To view, visit http://gerrit.cloudera.org:8080/13539 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie83a7d2497b778833f3f96fa8aa20e7486c5ebbb Gerrit-Change-Number: 13539 Gerrit-PatchSet: 3 Gerrit-Owner: Yifan Zhang <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Comment-Date: Fri, 07 Jun 2019 16:22:40 +0000 Gerrit-HasComments: Yes
