Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15970 )
Change subject: auto_rebalancer: ignore deleted tables ...................................................................... Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/15970/4/src/kudu/master/auto_rebalancer-test.cc File src/kudu/master/auto_rebalancer-test.cc: http://gerrit.cloudera.org:8080/#/c/15970/4/src/kudu/master/auto_rebalancer-test.cc@647 PS4, Line 647: Test rebalancing with deleted tables nit: it would be nice to add an extra sentence to explain what is the desired behavior of the auto-rebalancer w.r.t. deleted tables, like do we simply want to make sure it doesn't stall; doesn't attempt to move replicas of the deleted tables, etc.? http://gerrit.cloudera.org:8080/#/c/15970/4/src/kudu/master/auto_rebalancer-test.cc@685 PS4, Line 685: // Even though not all tables are running, this should succeed because the : // table that isn't running has been deleted. : ASSERT_EVENTUALLY([&] { : int bytes_sent_in_orig_tservers = : AggregateMetricCounts(GetBytesSentByTServer(), 0, kNumTServers); : ASSERT_GT(bytes_sent_in_orig_tservers, initial_bytes_sent_in_orig_tservers); : }); : ASSERT_EVENTUALLY([&] { : int bytes_fetched_in_new_tservers = : AggregateMetricCounts(GetBytesFetchedByTServer(), kNumTServers, : cluster_->num_tablet_servers()); : ASSERT_GT(bytes_fetched_in_new_tservers, 0); : }); Does it give a guarantee that the replicas of the deleted table were not attempted to be moved? Or this test isn't trying to verify that? http://gerrit.cloudera.org:8080/#/c/15970/4/src/kudu/master/auto_rebalancer.cc File src/kudu/master/auto_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/15970/4/src/kudu/master/auto_rebalancer.cc@498 PS4, Line 498: TableSummary table_summary; : table_summary.id = table->id(); : const SysTablesEntryPB& table_data = table->metadata().state().pb; : if (table_data.state() == SysTablesEntryPB::REMOVED) { : // Don't worry about rebalancing replicas that belong to deleted tables. : continue; : } nit: does it make sense to re-order the lines to avoid instantiating 'table_summary' at all for the short-circuit case? Something like: const SysTablesEntryPB& table_data = table->metadata().state().pb; if (table_data.state() == SysTablesEntryPB::REMOVED) { // Don't worry about rebalancing replicas that belong to deleted tables. continue; } TableSummary table_summary; table_summary.id = table->id(); -- To view, visit http://gerrit.cloudera.org:8080/15970 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib5499b09c677dd6c1016349398952a1c39820691 Gerrit-Change-Number: 15970 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Hannah Nguyen <hannahvnguye...@gmail.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 21 May 2020 02:24:46 +0000 Gerrit-HasComments: Yes
