Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19501 )
Change subject: [Tool] Show the information of a tablet ...................................................................... Patch Set 9: (7 comments) http://gerrit.cloudera.org:8080/#/c/19501/9/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: http://gerrit.cloudera.org:8080/#/c/19501/9/src/kudu/client/client-internal.cc@298 PS9, Line 298: Status KuduClient::Data::GetTablet(KuduClient* client, I'm not clear why to move this function from KuduClient to KuduClient::Data, it's possible to hide this function in private scope of KuduClient, right? http://gerrit.cloudera.org:8080/#/c/19501/9/src/kudu/client/client-internal.cc@301 PS9, Line 301: const At least, it can be static (in L307, use client->default_admin_operation_timeout() instead of default_admin_operation_timeout_) http://gerrit.cloudera.org:8080/#/c/19501/9/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/19501/9/src/kudu/master/catalog_manager.h@797 PS9, Line 797: get_extra_info Could you plz add some comments to describe what dose 'get_extra_info' mean? http://gerrit.cloudera.org:8080/#/c/19501/9/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/19501/9/src/kudu/master/catalog_manager.cc@6241 PS9, Line 6241: R nit: indent http://gerrit.cloudera.org:8080/#/c/19501/5/src/kudu/tools/tool_action_tablet.cc File src/kudu/tools/tool_action_tablet.cc: http://gerrit.cloudera.org:8080/#/c/19501/5/src/kudu/tools/tool_action_tablet.cc@101 PS5, Line 101: ShowTabletI > OK. I will use friend function. One of the advantages of using frined class v.s friend function is the class name is much shorter than the function signature. In the latest patch set, the too long friend function signature in client.h seems a bit of code smell :) cc @Alexey Serbin http://gerrit.cloudera.org:8080/#/c/19501/9/src/kudu/tools/tool_action_tablet.cc File src/kudu/tools/tool_action_tablet.cc: http://gerrit.cloudera.org:8080/#/c/19501/9/src/kudu/tools/tool_action_tablet.cc@109 PS9, Line 109: if (FLAGS_json) { DataTable is support to output in JSON format(see DataTable::PrintTo), why deal with the JSON format especially? http://gerrit.cloudera.org:8080/#/c/19501/9/src/kudu/tools/tool_action_tablet.cc@124 PS9, Line 124: cout << Substitute("Table id: $0, Table name: $1", So if --json is enabled, table id and name will not be output? How about add another CLI parameter --columns, and add table id and name to another output columns, like how dose 'kudu tserver list' do? -- To view, visit http://gerrit.cloudera.org:8080/19501 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib5ae5f61f50a44c4787843df76adaa61700ae9fe Gerrit-Change-Number: 19501 Gerrit-PatchSet: 9 Gerrit-Owner: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: KeDeng <kdeng...@gmail.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yingchun Lai <laiyingc...@apache.org> Gerrit-Reviewer: Yuqi Du <shenxingwuy...@gmail.com> Gerrit-Comment-Date: Wed, 22 Mar 2023 16:10:28 +0000 Gerrit-HasComments: Yes
