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 10: (8 comments) http://gerrit.cloudera.org:8080/#/c/18834/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18834/9//COMMIT_MSG@21 PS9, Line 21: ASH (key_hash0) PA > nit: would it better to use the default port 7050 in example? Done http://gerrit.cloudera.org:8080/#/c/18834/9//COMMIT_MSG@56 PS9, Line 56: > Is the command same with 'table' format above except the `-list_table_outpu Done http://gerrit.cloudera.org:8080/#/c/18834/9//COMMIT_MSG@61 PS9, Line 61: > nit: would it better to use some thing like 'tserver1:7050' instead? And it Done http://gerrit.cloudera.org:8080/#/c/18834/9/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/18834/9/src/kudu/tools/kudu-admin-test.cc@1694 PS9, Line 1694: > why use xxx_NE, would it better to check what it would be, but not what it Done http://gerrit.cloudera.org:8080/#/c/18834/9/src/kudu/tools/kudu-admin-test.cc@1716 PS9, Line 1716: return { "pretty", "json", "json_compact" }; : } : : class ListTableCliSimpleParamTest : : public AdminCliTest, : public ::testing::WithParamInterface<string> { : }; : : INSTANTIATE_TEST_SUITE_P(, ListTableCliSimpleParamTest, : ::testing::ValuesIn(TableListFormat())); : : TEST_P(ListTableCliSimpleParamTest, TestListTables) { : FLAGS_num_tablet_servers = 1; : FLAGS_num_replicas = 1; : : NO_FATALS(BuildAndStart()); : : > Why not use ::testing::WithParamInterface<string> directly? Done http://gerrit.cloudera.org:8080/#/c/18834/9/src/kudu/tools/kudu-admin-test.cc@1833 PS9, Line 1833: > How about use 'raw string literal' in C++11 to simplify the code? We need to embeds the string with specified string. The 'raw string literal'is not convient to use here. http://gerrit.cloudera.org:8080/#/c/18834/9/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/18834/9/src/kudu/tools/kudu-tool-test.cc@4744 PS9, Line 4744: } > how about use MatchPattern in gutil/strings/util.h? Done http://gerrit.cloudera.org:8080/#/c/18834/7/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/18834/7/src/kudu/tools/tool_action_table.cc@251 PS7, Line 251: > I meant we can use DataTable to unify the output of table/json/json_compact The reason for adding the format param is to facilitate the collection of JSON format data. I tried the DataTable, but it didn't work well. So I finally chose the compatible output format. -- 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: 10 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: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Tue, 23 Aug 2022 09:43:45 +0000 Gerrit-HasComments: Yes
