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 9: (7 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: 127.14.18.65:18773 nit: would it better to use the default port 7050 in example? http://gerrit.cloudera.org:8080/#/c/18834/9//COMMIT_MSG@56 PS9, Line 56: "partition_info": "", Is the command same with 'table' format above except the `-list_table_output` parameter? why 'list_table_output' details is not shown here? http://gerrit.cloudera.org:8080/#/c/18834/9//COMMIT_MSG@61 PS9, Line 61: 127.24.68.66:32119 nit: would it better to use some thing like 'tserver1:7050' instead? And it'd better to keep consistency with other output in different format, only the format is variant, then reviewers/readers woule be easier to understand your intend. 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: ASSERT_NE why use xxx_NE, would it better to check what it would be, but not what it would not be? http://gerrit.cloudera.org:8080/#/c/18834/9/src/kudu/tools/kudu-admin-test.cc@1716 PS9, Line 1716: string ParamsToOutputFormat(const TableListFormat& param_type) { : string output_format; : switch (param_type) { : case TableListFormat::TABLE: : output_format = "table"; : break; : case TableListFormat::JSON: : output_format = "json"; : break; : case TableListFormat::JSON_COMPACT: : output_format = "json_compact"; : break; : default: : LOG(FATAL) << "unknown TableListFormat"; : } : : return output_format; : } Why not use ::testing::WithParamInterface<string> directly? http://gerrit.cloudera.org:8080/#/c/18834/9/src/kudu/tools/kudu-admin-test.cc@1833 PS9, Line 1833: "\"name\": \"$0\"" How about use 'raw string literal' in C++11 to simplify the code? 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: if (lines[i].find("kudu.table_") == string::npos) { how about use MatchPattern in gutil/strings/util.h? -- 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: 9 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: Mon, 22 Aug 2022 03:26:43 +0000 Gerrit-HasComments: Yes
