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 10: (18 comments) http://gerrit.cloudera.org:8080/#/c/18834/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18834/10//COMMIT_MSG@9 PS10, Line 9: table > pretty ? Done http://gerrit.cloudera.org:8080/#/c/18834/10//COMMIT_MSG@10 PS10, Line 10: Default > nit: default Done http://gerrit.cloudera.org:8080/#/c/18834/10//COMMIT_MSG@10 PS10, Line 10: Default > nit: default Done http://gerrit.cloudera.org:8080/#/c/18834/10//COMMIT_MSG@11 PS10, Line 11: convenient > nit: conveniently Done http://gerrit.cloudera.org:8080/#/c/18834/10//COMMIT_MSG@11 PS10, Line 11: more convenient > nit: more convenient for automatic processing Done http://gerrit.cloudera.org:8080/#/c/18834/10//COMMIT_MSG@11 PS10, Line 11: the scenario of large-scale cluster > int: in case of large-scale clusters Done http://gerrit.cloudera.org:8080/#/c/18834/10//COMMIT_MSG@14 PS10, Line 14: prety > nit: pretty Done http://gerrit.cloudera.org:8080/#/c/18834/10//COMMIT_MSG@16 PS10, Line 16: prety > pretty Done http://gerrit.cloudera.org:8080/#/c/18834/10//COMMIT_MSG@16 PS10, Line 16: prety > nit: pretty Done http://gerrit.cloudera.org:8080/#/c/18834/10//COMMIT_MSG@52 PS10, Line 52: "num_tablets": 12, > nit: maybe, create a table with less tablets to have more compact example i Done 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: list_table_output > That's about the output format for tablets, right? Maybe, let's then name This param works for all the output of 'kudu table list', not only the output for the tablets. http://gerrit.cloudera.org:8080/#/c/18834/10/src/kudu/tools/tool_action_table.cc@155 PS10, Line 155: bool ValidateListTableOutput() { : if (!iequals(FLAGS_list_table_output, "pretty") && !iequals(FLAGS_list_table_output, "json") : && !iequals(FLAGS_list_table_output, "json_compact")) { : LOG(ERROR) << Substitute("--list_table_output should be one of 'json', 'json_compact'" : " or 'pretty'."); : return false; : } : return true; : } : : GROUP_FLAG_VALIDATOR(list_table_output, ValidateListTableOutput); > Since this validator involves only a single flag, you don't need to have GR Done http://gerrit.cloudera.org:8080/#/c/18834/10/src/kudu/tools/tool_action_table.cc@256 PS10, Line 256: if (!table_info.has_name()) { : continue; : } > When is it possible to have no name/id for a table? Would be great to add Done http://gerrit.cloudera.org:8080/#/c/18834/10/src/kudu/tools/tool_action_table.cc@260 PS10, Line 260: need_linefeed > nit: is it possible to rely on !output.empty() instead? Done http://gerrit.cloudera.org:8080/#/c/18834/10/src/kudu/tools/tool_action_table.cc@261 PS10, Line 261: string( > nit: I guess it's possible to drop conversion to string and rely on std::st Done http://gerrit.cloudera.org:8080/#/c/18834/10/src/kudu/tools/tool_action_table.cc@265 PS10, Line 265: output += string(" num_tablets:") + std::to_string(table_info.num_tablets()) + : string(" num_replicas:") + std::to_string(table_info.num_replicas()) + : string(" live_row_count:") + std::to_string(table_info.live_row_count()); > Instead, is it possible to use strings::Substitute() to build the string to Done http://gerrit.cloudera.org:8080/#/c/18834/10/src/kudu/tools/tool_action_table.cc@271 PS10, Line 271: if (!tablet_info.has_tablet_id()) { : continue; : } > When is it possible to have a situation when no identifier for a tablet is Done http://gerrit.cloudera.org:8080/#/c/18834/10/src/kudu/tools/tool_action_table.cc@283 PS10, Line 283: output += string("\n") + : string(" ") + replica_info.role() + : string(" ") + replica_info.uuid() + : string(" ") + replica_info.host_port(); > Instead, is it possible to use strings::Substitute() to build the string to 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: 10 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: Fri, 26 Aug 2022 03:22:53 +0000 Gerrit-HasComments: Yes
