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 16: (6 comments) http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/client/client-test.cc@8539 PS10, Line 8539: client_->data_->meta_cache_->ClearCache(); > Thanks Alexey, Andrew, I add some test cases in src/kudu/integration-tests/ Thank you for adding these! I'm fine with this test being here. 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: CHECK(tablet_replica_); : } : LOG(INFO) << "Tablet successfully located"; : } : : void TearDown() override { : tablet_replica_.reset(); : cluster_->Shutdown(); : } : : scoped_refptr<TabletReplica> LookupLeaderTabletReplica(const MonoDelta& timeout) { : 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)); : con > Sure, though keep in mind methods like cluster_->mini_tablet_server_by_uuid Coming back to this, I actually don't think there'd be much refactoring. It'd just be a matter of calling those functions and getting the tablet replica from the fetched UUID. It'd be nice to reuse some the utilities so we don't have several implementations dedicated to finding leaders: TabletServerMap ts_map; TServerDetails* leader; ASSERT_OK(CreateTabletServerMap(cluster_->master_proxy(), cluster_->messenger(), &ts_map); ASSERT_OK(FindTabletLeader(ts_map, replicas[0]->tablet_id(), &leader)); cluster_->mini_tablet_server_by_uuid(leader->uuid())->server()->tablet_manager()->GetTabletReplicas... http://gerrit.cloudera.org:8080/#/c/17674/16/src/kudu/integration-tests/alter_table-test.cc File src/kudu/integration-tests/alter_table-test.cc: http://gerrit.cloudera.org:8080/#/c/17674/16/src/kudu/integration-tests/alter_table-test.cc@2265 PS16, Line 2265: nthe nit: missing a space http://gerrit.cloudera.org:8080/#/c/17674/16/src/kudu/integration-tests/alter_table-test.cc@2299 PS16, Line 2299: aning nit: Scanning http://gerrit.cloudera.org:8080/#/c/17674/16/src/kudu/integration-tests/alter_table-test.cc@2355 PS16, Line 2355: = 100; I'm curious, if you set this to something very high, would we be able to see some write failures? I think that would be a fine limitation, especially given some tablet copies may be huge and it seems reasonable to expect such a heavy-weight copy to affect workloads. It would be good to know if that happens. http://gerrit.cloudera.org:8080/#/c/17674/16/src/kudu/integration-tests/alter_table-test.cc@2377 PS16, Line 2377: Not nit: Now? -- 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: 16 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Wed, 18 Aug 2021 18:29:27 +0000 Gerrit-HasComments: Yes
