KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18834 )
Change subject: [tool] Add output format for 'table list' CLI tool. ...................................................................... Patch Set 12: (5 comments) http://gerrit.cloudera.org:8080/#/c/18834/11/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/18834/11/src/kudu/tools/kudu-admin-test.cc@1715 PS11, Line 1715: vector<string> TableList > nit: "std::" can be omitted. Done http://gerrit.cloudera.org:8080/#/c/18834/11/src/kudu/tools/kudu-admin-test.cc@2272 PS11, Line 2272: const auto show_tp = std::get<0>(GetParam()) ? PartitionSchema::HashPartitionInfo::SHOW : : PartitionSchema::HashPartitionInfo::HIDE; : const auto show_hp = std::get<1>(GetParam()) ? PartitionSchema::HashPartitionInfo::SHOW : : PartitionSchema::HashPartitionInfo::HIDE; > show_tp and show_hp are bool type. Done http://gerrit.cloudera.org:8080/#/c/18834/11/src/kudu/tools/kudu-admin-test.cc@2349 PS11, Line 2349: Substitute("--show_tablet_partition_info=$0", std::get<0>(GetParam())), : Substitute("--show_hash_partition_info=$0", std::get<1>(GetParam())), > nit: I think the former style is more simple, why change it ? You mean '--show_tablet_partition_info', right? The reason I add 'Substitute' is to fit the 'GetParam()' that make testing easier. http://gerrit.cloudera.org:8080/#/c/18834/11/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/18834/11/src/kudu/tools/kudu-tool-test.cc@4745 PS11, Line 4745: lines[i].erase(0, lines[i].find_first_not_of(' ')); > nit: 'lines[i].erase(0, lines[i].find_first_not_of(' '));' Done http://gerrit.cloudera.org:8080/#/c/18834/11/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/18834/11/src/kudu/tools/tool_action_table.cc@273 PS11, Line 273: table_info.num_tablets(), : table_info.num_replicas(), : table_info.live_row_count()); > nit: no need to use `std::to_string`, Substitute will deal with it correctl Done -- To view, visit http://gerrit.cloudera.org:8080/18834 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3aaec73e18872fc93646e9c0ea675b578b0702f0 Gerrit-Change-Number: 18834 Gerrit-PatchSet: 12 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Mon, 29 Aug 2022 02:53:35 +0000 Gerrit-HasComments: Yes
