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 11: (11 comments) http://gerrit.cloudera.org:8080/#/c/17674/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17674/10//COMMIT_MSG@13 PS10, Line 13: add > nit: adds Done http://gerrit.cloudera.org:8080/#/c/17674/10//COMMIT_MSG@13 PS10, Line 13: supported : in CLI tool > Could you add a small blurb to explain that the CLI tool returns without wa Done 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(); > I'm curious how this works for meta cache of other clients which might be w The tablet cache entry has a TTL (src/kudu/client/meta_cache.h:379), when the cache is expired, the client will ask master to request the latest schame. And also the cache will be updated when occured errors, see MetaCacheServerPicker::PickLeader(). I think the procedure of increase RF is a kind of 'replica under replicated', the master will add new replicas, and consensus mechanism will ensure data to be replicated. And it's not cient's responsibility to replicate data, it's raft leader peer's responsibility. http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/integration-tests/alter_table-test.cc File src/kudu/integration-tests/alter_table-test.cc: http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/integration-tests/alter_table-test.cc@289 PS10, Line 289: first_n > nit: maybe call this "first_replica" or something more descriptive. Done http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/integration-tests/alter_table-test.cc@296 PS10, Line 296: T_EQ(1, cur > nit: maybe assign this a variable name, like cur_node_replica or something. Done http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/integration-tests/alter_table-test.cc@305 PS10, Line 305: ASSERT_TRUE(first_node_replica->tablet > nit: we could compute this once for the first replica after L299. Also mayb Done http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/integration-tests/alter_table-test.cc@2319 PS10, Line 2319: // Wait util some WALs have been GCed. : ASSERT_EVENTUALLY([&] { : int num_tserver_gc_updated = 0; : for (int tserver_idx = 0; tserver_idx < num_tservers(); tserver_idx++) { : if (get_tablet_wal_gc_count(tserver_idx) > orig_gc_count[tserver_idx]) { : num_tserver_gc_updated++; : } : } : > nit: seems like this is called once -- just inline it instead of defining a Done http://gerrit.cloudera.org:8080/#/c/17674/4/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17674/4/src/kudu/master/catalog_manager.cc@3062 PS4, Line 3062: "table name $0 is already reserved > Ah, it seems this one is always a derivative of 'has_metadata_changes' and Ack http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/master/catalog_manager.cc@262 PS10, Line 262: facto > nit: factor Done http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/tools/tool_action_table.cc@988 PS10, Line 988: : return TableAlter::SetReplicationFactor(master_addresses, table_name, replication_factor); : } : > It seems this check is redundant since the minimum value of the replication Indeed, I’ll remove it. http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/tools/tool_action_table.cc@994 PS10, Line 994: client::sp::shared_ptr<KuduClient> client; > nit: an extra line? 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: 11 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: Sat, 07 Aug 2021 04:43:35 +0000 Gerrit-HasComments: Yes
