Alexey Serbin 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 14: (7 comments) http://gerrit.cloudera.org:8080/#/c/18834/14//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18834/14//COMMIT_MSG@10 PS14, Line 10: table pretty ? http://gerrit.cloudera.org:8080/#/c/18834/14//COMMIT_MSG@42 PS14, Line 42: : What's the significance of having this in the field? Is it possible to get rid of this part in the JSON and JSON_COMPACT formats? (sorry: I didn't notice that in prior review rounds). http://gerrit.cloudera.org:8080/#/c/18834/10/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/18834/10/src/kudu/tools/tool_action_table.cc@151 PS10, Line 151: "Leave empty to r > This param works for all the output of 'kudu table list', not only the outp Ah, indeed. Then maybe let's name this flag 'list_table_format' or 'list_table_output_format'? We do have '--ksck_format' and '--format' flags for other CLI tools already. http://gerrit.cloudera.org:8080/#/c/18834/14/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/18834/14/src/kudu/tools/tool_action_table.cc@96 PS14, Line 96: ostream nit: should this be 'ostringstream' instead? http://gerrit.cloudera.org:8080/#/c/18834/14/src/kudu/tools/tool_action_table.cc@159 PS14, Line 159: std:: nit: below the 'copy' algorithm is used without the 'std::' prefix; maybe add corresponding 'using ...' and get rid of other 'std::' prefixes below? http://gerrit.cloudera.org:8080/#/c/18834/14/src/kudu/tools/tool_action_table.cc@166 PS14, Line 166: std:: nit: if adding 'using std::ostringstream', the std:: prefix might be omitted http://gerrit.cloudera.org:8080/#/c/18834/14/src/kudu/tools/tool_action_table.cc@358 PS14, Line 358: auto mode = iequals(FLAGS_list_table_output, "json") ? nit: since the validator for --list_table_output might be changed independently, does it make sense to add DCHECK(iequals(FLAGS_list_table_output, "json") || iequals(FLAGS_list_table_output, "json_compact")) here? -- 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: 14 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, 03 Sep 2022 05:16:32 +0000 Gerrit-HasComments: Yes
