Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18834 )
Change subject: [tool] Add output format for tabele list cli tool. ...................................................................... Patch Set 1: (8 comments) Thank you for the patch! http://gerrit.cloudera.org:8080/#/c/18834/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18834/1//COMMIT_MSG@7 PS1, Line 7: tabele list 'table list' http://gerrit.cloudera.org:8080/#/c/18834/1//COMMIT_MSG@7 PS1, Line 7: cli CLI http://gerrit.cloudera.org:8080/#/c/18834/1/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/18834/1/src/kudu/tools/kudu-admin-test.cc@1802 PS1, Line 1802: ASSERT_TRUE(false); Maybe, use FAIL() instead? Also, print out the information why test fails in such case, something like FAIL() << "unexpected table list format" << static_cast<uint16_t>(GetParam()); http://gerrit.cloudera.org:8080/#/c/18834/1/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/18834/1/src/kudu/tools/tool.proto@517 PS1, Line 517: required Change this into 'optional': we hope to migrate to protobuf3 at some point http://gerrit.cloudera.org:8080/#/c/18834/1/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/18834/1/src/kudu/tools/tool_action_table.cc@262 PS1, Line 262: "table" Does it make sense to introduce constants for these output types? http://gerrit.cloudera.org:8080/#/c/18834/1/src/kudu/tools/tool_action_table.cc@272 PS1, Line 272: } else { : if (FLAGS_list_table_output == "table") { : cout << tname << endl; : } else { : table_info_pb->set_name(tname); : } : } Instead of outputting for the 'table' format here, maybe just accumulate the information into the TablesInfoPB structure here as well and also introduce a function that prints out TablesInfoPB structure. Then use that function in one place, somewhere around line 328 -- 330 where JSON-based data is printed out as well? http://gerrit.cloudera.org:8080/#/c/18834/1/src/kudu/tools/tool_action_table.cc@329 PS1, Line 329: iequals(FLAGS_list_table_output, "json") If comparing in a case-insensitive way for the output type here, do the same in every other place (like validation of the input for the flags, etc.). http://gerrit.cloudera.org:8080/#/c/18834/1/src/kudu/tools/tool_test_util.h File src/kudu/tools/tool_test_util.h: http://gerrit.cloudera.org:8080/#/c/18834/1/src/kudu/tools/tool_test_util.h@36 PS1, Line 36: enum class TableListFormat { : table, : json, : json_compact, : }; I'm not sure that's the best place for this definition. Maybe, add that into the tool_action.h file instead? Also, per code style guide, capital letters should be used for enumerator tags. -- 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: 1 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 12 Aug 2022 01:58:53 +0000 Gerrit-HasComments: Yes
