Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17674 )
Change subject: KUDU-3304: [Alter] Support to alter table's replication factor ...................................................................... Patch Set 4: Code-Review+1 (2 comments) Just a couple testing nits. http://gerrit.cloudera.org:8080/#/c/17674/4/src/kudu/integration-tests/alter_table-test.cc File src/kudu/integration-tests/alter_table-test.cc: http://gerrit.cloudera.org:8080/#/c/17674/4/src/kudu/integration-tests/alter_table-test.cc@173 PS4, Line 173: std::unique_ptr<ThreadPool> pool; : CHECK_OK(ThreadPoolBuilder("WaitUntilLeader.Pool") : .set_max_threads(num_tservers()) : .set_idle_timeout(MonoDelta::FromMilliseconds(10)) : .Build(&pool)); : scoped_refptr<TabletReplica> leader_replica; : std::atomic<bool> got_leader(false); : for (int i = 0; i < num_tservers(); ++i) { : CHECK_OK(pool->Submit( : [this, i, &got_leader, &leader_replica]() { : int loop = 10; : while (!got_leader && loop > 0) { : vector<scoped_refptr<TabletReplica> > replicas; : cluster_->mini_tablet_server(i)->server()->tablet_manager() : ->GetTabletReplicas(&replicas); : if (!replicas.empty()) { : auto s = replicas[0]->consensus() : ->WaitUntilLeaderForTests(MonoDelta::FromSeconds(1)); : if (s.ok()) { : got_leader = true; : leader_replica = replicas[0]; : break; : } : } : --loop; : } : })); : } : pool->Wait(); nit: perhaps consider using CreateTabletServerMap() and FindTabletLeader() from integration-tests/cluster_itest_util.cc to get the leader server, and then fetch the replica? http://gerrit.cloudera.org:8080/#/c/17674/4/src/kudu/integration-tests/alter_table-test.cc@272 PS4, Line 272: int loop = 10; : bool peers_count_ok = false; : while (!peers_count_ok && loop-- > 0) { : if (replication_factor != tablet_replica_->consensus()->CommittedConfig().peers().size()) { : SleepFor(MonoDelta::FromMilliseconds(500)); : continue; : } : : int actual_replica_count = 0; : scoped_refptr<TabletReplica> replica; : for (int i = 0; i < num_tservers(); ++i) { : vector<scoped_refptr<TabletReplica>> replicas; : cluster_->mini_tablet_server(i)->server()->tablet_manager()->GetTabletReplicas(&replicas); : if (replicas.empty()) { : continue; : } : ASSERT_EQ(1, replicas.size()); : if (!replicas[0]->tablet()) { : continue; : } : if (!replica) { : replica = replicas[0]; : } else { : ASSERT_EQ(replica->tablet()->tablet_id(), replicas[0]->tablet()->tablet_id()); : ASSERT_TRUE(replica->tablet()->schema()->Equals(*(replicas[0]->tablet()->schema()))); : } : ++actual_replica_count; : } : : if (replication_factor != actual_replica_count) { : SleepFor(MonoDelta::FromMilliseconds(500)); : continue; : } : : peers_count_ok = true; : } : ASSERT_TRUE(peers_count_ok); nit: rather than looping, maybe consider wrapping the body in an ASSERT_EVENTUALLY() macro? It's a bit more common in our codebase, and time-based checking here seems like it might be more robust to flakiness in the future. E.g. ASSERT_EVENTUALLY([&] { int actual_replica_count = 0; for (int i = 0; i < num_tservers(); i++) { vector<scoped_refptr<TabletReplica>> replicas; cluster_->mini_tablet_server(i)->server()->tablet_manager()->GetTabletReplicas(&replicas); if (replicas.empty()) continue; ASSERT_EQ(1, replicas.size()); if (!replica) ... ++actual_replica_count; } ASSERT_EQ(replication_factor, actual_replica_count); }); -- To view, visit http://gerrit.cloudera.org:8080/17674 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3aa2d5b12c508ba761fa9410ad1a465cf31fb7d7 Gerrit-Change-Number: 17674 Gerrit-PatchSet: 4 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Wed, 14 Jul 2021 01:15:37 +0000 Gerrit-HasComments: Yes
