[kudu-CR] [tool] Add output format for 'table list' CLI tool.

["Alexey Serbin (Code Review)" <ge...@cloudera.org>]

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 <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Sat, 03 Sep 2022 05:16:32 +0000
Gerrit-HasComments: Yes