Yingchun Lai 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 6: (8 comments) 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: scoped_refptr<TabletReplica> leader_replica; : MonoTime deadline = MonoTime::Now() + timeout; : std::unique_ptr<ThreadPool> pool; : CHECK_OK(ThreadPoolBuilder("WaitUntilLeader.Pool") : .set_max_threads(num_tservers()) : .set_idle_timeout(MonoDelta::FromMilliseconds(10)) : .Build(&pool)); : std::atomic<bool> got_leader(false); : for (int i = 0; i < num_tservers(); ++i) { : CHECK_OK(pool->Submit( : [this, i, deadline, &got_leader, &leader_replica]() { : while (!got_leader && MonoTime::Now() < deadline) { : vector<scoped_refptr<TabletReplica> > replicas; : cluster_->mini_tablet_server(i)->server()->tablet_manager() : ->GetTabletReplicas(&replicas); : if (replicas.empty()) { : SleepFor(MonoDelta::FromMilliseconds(50)); : continue; : } : : MonoDelta remaining_timeout = deadline - MonoTime::Now(); : auto s = replicas[0]->consensus()->WaitUntilLeaderForTests(remaining_timeout); : if (!s.ok()) { : SleepFor(MonoDelta::FromMilliseconds(50)); : continue; : } : : got_leader = true; : leade > nit: perhaps consider using CreateTabletServerMap() and FindTabletLeader() I improved these code a little. And seems not suitable to use CreateTabletServerMap / FindTabletLeader, the 2 functions are operating on *Proxy, which is on a client view, but this class are using *Server itself, the will be much code refactor if we do that. http://gerrit.cloudera.org:8080/#/c/17674/4/src/kudu/integration-tests/alter_table-test.cc@272 PS4, Line 272: return table_alterer->timeout(MonoDelta::FromSeconds(60))->Alter(); : } : : void VerifyTabletReplicaCount(int32_t replication_factor) { : ASSERT_EVENTUALLY([&] { : ASSERT_EQ(replication_factor, tablet_replica_->consensus()->CommittedConfig().peers().size()); : : scoped_refptr<TabletReplica> replica; : 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 (!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; : } : ASSERT_EQ(replication_factor, actual_replica_count); : }); : } : : enum VerifyPattern { : C1_MATCHES_INDEX, : C1_IS_DEADBEEF, : C1_DOESNT_EXIST : }; : : void VerifyRows(int start_row, int num_rows, VerifyPattern pattern); : : void InsertRows(int start_row, > nit: rather than looping, maybe consider wrapping the body in an ASSERT_EVE Done http://gerrit.cloudera.org:8080/#/c/17674/4/src/kudu/integration-tests/alter_table-test.cc@2242 PS4, Line 2242: b > nit: -1 Done http://gerrit.cloudera.org:8080/#/c/17674/4/src/kudu/integration-tests/alter_table-test.cc@2265 PS4, Line 2265: NO_FATALS(VerifyTabletReplicaCount(3)); > It would be great to add a sub-scenario to call SetReplicationFactor(3) whe Done http://gerrit.cloudera.org:8080/#/c/17674/4/src/kudu/integration-tests/alter_table-test.cc@2266 PS4, Line 2266: ASSERT_E > ASSERT_OK() seems to be a better choice aligned with the rest of the code i It should be CHECK_OK() :) http://gerrit.cloudera.org:8080/#/c/17674/4/src/kudu/integration-tests/alter_table-test.cc@2267 PS4, Line 2267: > Do you mind adding a test scenario to verify the case when trying to set re Done http://gerrit.cloudera.org:8080/#/c/17674/4/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/17674/4/src/kudu/tools/kudu-tool-test.cc@4482 PS4, Line 4482: constexpr const char* co > nit: could could be a constexrp: Done http://gerrit.cloudera.org:8080/#/c/17674/4/src/kudu/tools/kudu-tool-test.cc@4546 PS4, Line 4546: " requested replication factor 5; 4 tablet servers are alive"); > Does it make sense to add a scenario to check setting replication factor to Done -- 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: 6 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: Thu, 15 Jul 2021 05:05:43 +0000 Gerrit-HasComments: Yes
