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 2: (8 comments) 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: CL > CLI Done http://gerrit.cloudera.org:8080/#/c/18834/1//COMMIT_MSG@7 PS1, Line 7: 'table list > 'table list' Done 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: FAIL() << "unexpect > Maybe, use FAIL() instead? Also, print out the information why test fails Done 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: optional > Change this into 'optional': we hope to migrate to protobuf3 at some point Done 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: > Does it make sense to introduce constants for these output types? We do need to format the output in actual use. In order not to break our usage habits, constant distinction is introduced. Do you have a better idea? http://gerrit.cloudera.org:8080/#/c/18834/1/src/kudu/tools/tool_action_table.cc@272 PS1, Line 272: : } : output += string("\n"); : } : return output; : } : > Instead of outputting for the 'table' format here, maybe just accumulate th Done http://gerrit.cloudera.org:8080/#/c/18834/1/src/kudu/tools/tool_action_table.cc@329 PS1, Line 329: ol is_voter = ReplicaController::is_vote > If comparing in a case-insensitive way for the output type here, do the sam Done 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: // Get full path to the top-level 'kudu' tool binary. : std::string GetKuduToolAbsolutePath(); : : // Runs the 'kudu' tool binary with the given arguments. : // > I'm not sure that's the best place for this definition. Maybe, add that in Done -- 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: 2 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 12 Aug 2022 07:50:55 +0000 Gerrit-HasComments: Yes
