Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19501 )
Change subject: [Tool] Show the information of a tablet ...................................................................... Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/19501/3/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/19501/3/src/kudu/client/client.h@1180 PS3, Line 1180: const std::string& table_id() const; : const std::string& table_name() const; I don't think these new methods are needed in the public client API if only kudu CLI tools use these. Wouldn't it be enough to have corresponding fields in KuduTablet::Data? For a reference, please take a look at how kudu::tools::TableLister uses internal::ReplicaController::Visibility to expose non-voter replicas in kudu CLI tools. http://gerrit.cloudera.org:8080/#/c/19501/3/src/kudu/master/master.proto File src/kudu/master/master.proto: PS3: The newly added fields TabletLocationsPB::table_id and TabletLocationsPB::table_name isn't needed anywhere but only in the newly added tool. If added to every response, these new fields will increase the size of GetTabletLocationsResponsePB and GetTableLocationsResponsePB. The latter might increase in size significantly, and the information in the newly added fields is duplicated, and it has no value at all for the callers of GetTableLocationsResponsePB. That would add unnecessary load to the kudu-master processes whose scalability is an issue for large clusters (200+ nodes) already. I think the new fields should be populated conditionally, and only the newly added tools should request these to be filled in. Consider adding a new boolean field similar to GetTabletLocationsRequestPB::intern_ts_infos_in_response, and set it only when necessary. Probably, it would be necessary to add a new method into KuduClient::Data() interface, and use it in the CLI tools and reuse in the implementation of KuduClient::GetTablet(). Basically, the implementation of KuduClient::GetTablet() should be moved into KuduClient::Data::GetTablet() and modified, and KuduClient::GetTablet() would simply call KuduClient::Data::GetTablet() with corresponding parameters. http://gerrit.cloudera.org:8080/#/c/19501/3/src/kudu/tools/tool_action_tablet.cc File src/kudu/tools/tool_action_tablet.cc: http://gerrit.cloudera.org:8080/#/c/19501/3/src/kudu/tools/tool_action_tablet.cc@409 PS3, Line 409: show_info > what about 'describe' ? Alternatively, it could be just 'info'? -- 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: 3 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Sat, 25 Feb 2023 02:46:26 +0000 Gerrit-HasComments: Yes
