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 10: Code-Review+1 (5 comments) http://gerrit.cloudera.org:8080/#/c/17674/4/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/17674/4/src/kudu/client/client.h@1721 PS4, Line 1721: const PartitionSchema& partition_schema, : const std::map<std::string, std::string>& extra_configs); : : // Owned. : Data* data_; : : DISALLOW_COPY_AND_ASSIGN(KuduTable); : }; > Thanks for your suggestion, not expose to C++ client API and just use inter Thank you for making these changes! I appreciate it. 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: replica nit: maybe call this "first_replica" or something more descriptive. http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/integration-tests/alter_table-test.cc@296 PS10, Line 296: replicas[0] nit: maybe assign this a variable name, like cur_node_replica or something. In my opinion it'd be easier to understand with a descriptive name, vs comparing "replica" and "replicas[0]". http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/integration-tests/alter_table-test.cc@305 PS10, Line 305: ASSERT_OK(replica->CountLiveRows(&l)); nit: we could compute this once for the first replica after L299. Also maybe call it "first_count" or something more descriptive so it's easier to read. http://gerrit.cloudera.org:8080/#/c/17674/10/src/kudu/integration-tests/alter_table-test.cc@2319 PS10, Line 2319: auto tserver_updated_wal_gc_count = [&] (int least_tserver_count) { : 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++; : } : } : return num_tserver_gc_updated >= least_tserver_count; : }; nit: seems like this is called once -- just inline it instead of defining a lambda? -- 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: 10 Gerrit-Owner: Yingchun Lai <acelyc1112...@gmail.com> Gerrit-Reviewer: Abhishek Chennaka <achenn...@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: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <acelyc1112...@gmail.com> Gerrit-Comment-Date: Fri, 06 Aug 2021 07:07:34 +0000 Gerrit-HasComments: Yes
