Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/19501 )
Change subject: [Tool] Show the information of a tablet ...................................................................... Patch Set 15: (8 comments) http://gerrit.cloudera.org:8080/#/c/19501/15/src/kudu/client/client-internal.h File src/kudu/client/client-internal.h: http://gerrit.cloudera.org:8080/#/c/19501/15/src/kudu/client/client-internal.h@103 PS15, Line 103: internal::RemoteTabletServer** ts) const; This change is not matter with this patch, so you can fix it at another patch http://gerrit.cloudera.org:8080/#/c/19501/15/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: http://gerrit.cloudera.org:8080/#/c/19501/15/src/kudu/client/client-internal.cc@293 PS15, Line 293: const This change is not matter with this patch, so you can fix it at another patch http://gerrit.cloudera.org:8080/#/c/19501/15/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/19501/15/src/kudu/master/catalog_manager.h@800 PS15, Line 800: bool get_extra_info what about move this parameters to GetTabletLocations, give it a default value false? http://gerrit.cloudera.org:8080/#/c/19501/15/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/19501/15/src/kudu/master/master.proto@459 PS15, Line 459: optional ListTablesResponsePB.TableInfo table_info = 8; move this line to the end? and writing some comments is better. GetTabletLocationsResponsePB in GetTabletLocationsResponsePB is repeated, so this line seens move to GetTabletLocationsResponsePB is better to reduce duplicated infomation. http://gerrit.cloudera.org:8080/#/c/19501/15/src/kudu/tools/tool_action_tablet.cc File src/kudu/tools/tool_action_tablet.cc: http://gerrit.cloudera.org:8080/#/c/19501/15/src/kudu/tools/tool_action_tablet.cc@a92 PS15, Line 92: keep this empty line? http://gerrit.cloudera.org:8080/#/c/19501/15/src/kudu/tools/tool_action_tablet.cc@76 PS15, Line 76: using kudu::master::TSInfoPB; : using kudu::master::GetTabletLocationsRequestPB; : using kudu::master::GetTabletLocationsResponsePB; : using kudu::master::ReplaceTabletRequestPB; : using kudu::master::ReplaceTabletResponsePB; nit: order using kudu::master::GetTabletLocationsRequestPB; using kudu::master::GetTabletLocationsResponsePB; using kudu::master::ReplaceTabletRequestPB; using kudu::master::ReplaceTabletResponsePB; using kudu::master::TSInfoPB; http://gerrit.cloudera.org:8080/#/c/19501/15/src/kudu/tools/tool_action_tablet.cc@319 PS15, Line 319: deprecated_replicas Don't use 'deprecated_replicas', it's an old deprecated historical field. using interned_replicas instead if need. interned_replicas or resp.tablet_locations has no TsInfo, interned_replicas has the TsInfo index. You can use this to construct the response. http://gerrit.cloudera.org:8080/#/c/19501/15/src/kudu/tools/tool_action_tablet.cc@415 PS15, Line 415: "info", &Info What about 'describe', DescribeTablet? -- 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: 15 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: Fri, 28 Apr 2023 08:04:34 +0000 Gerrit-HasComments: Yes
