Yingchun Lai 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 11: (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: std::vector<std::string> nit: "std::" can be omitted. http://gerrit.cloudera.org:8080/#/c/18834/11/src/kudu/tools/kudu-admin-test.cc@2272 PS11, Line 2272: auto show_tp = std::get<0>(GetParam()) ? PartitionSchema::HashPartitionInfo::SHOW : : PartitionSchema::HashPartitionInfo::HIDE; : auto show_hp = std::get<1>(GetParam()) ? PartitionSchema::HashPartitionInfo::SHOW : : PartitionSchema::HashPartitionInfo::HIDE; show_tp and show_hp are bool type. 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 ? 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(' '));' 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: std::to_string(table_info.num_tablets()), : std::to_string(table_info.num_replicas()), : std::to_string(table_info.live_row_count()) nit: no need to use `std::to_string`, Substitute will deal with it correctlly? -- 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: 11 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: Sat, 27 Aug 2022 08:44:51 +0000 Gerrit-HasComments: Yes
