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 5: (7 comments) Almost there: just few more items to address. Thank you for working on this! http://gerrit.cloudera.org:8080/#/c/19501/5/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/19501/5/src/kudu/master/master.proto@460 PS5, Line 460: intern_ts_infos_in_response I guess this field rather belongs to GetTabletLocationsRequestPB? Also, consider naming this just 'table_info_in_response'. http://gerrit.cloudera.org:8080/#/c/19501/5/src/kudu/master/master.proto@461 PS5, Line 461: optional bytes table_id = 9; : optional string table_name = 10; It would be great to create a dedicated message for this and add it just as a single field 'table_info' into this TabletLocationsPB. E.g.: message TabletLocationsPB { ... message TableInfoPB { optional bytes id = 1; optional string name = 2; } ... optional TableInfoPB table_info = 8; ... } 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: ilable.\n > seems no changes about this? +1 It seems the corresponding update is missing: it's still names 'show_info' in PS5. Consider updating to 'describe' or 'info', as suggested. 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: TabletShower This is C++, not Java, so I don't understand why to introduce otherwise useless class just to have a function or method. C++ allows to declare a friend function as well, see https://en.cppreference.com/w/cpp/language/friend for the details. It can be even a template one: for an example, take a look at https://github.com/apache/kudu/blob/8ee73765fb78c246d3455029cdf02e15eac28926/src/kudu/util/once.h#L105-L106 BTW, 'TabletShower' is a bit funny if looking at it without much context, see https://en.wikipedia.org/wiki/Shower :) http://gerrit.cloudera.org:8080/#/c/19501/5/src/kudu/tools/tool_action_tablet.cc@126 PS5, Line 126: ostream out(std::cout.rdbuf()); Why would you need this? std::cout is std::ostream as well, so it fits where std::ostream is needed. http://gerrit.cloudera.org:8080/#/c/19501/5/src/kudu/tools/tool_action_tablet.cc@127 PS5, Line 127: out Would it be enough using 'cout' here? http://gerrit.cloudera.org:8080/#/c/19501/5/src/kudu/tools/tool_action_tablet.cc@136 PS5, Line 136: nit: wrong indent? -- 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: 5 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: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yingchun Lai <laiyingc...@apache.org> Gerrit-Reviewer: Yuqi Du <shenxingwuy...@gmail.com> Gerrit-Comment-Date: Tue, 14 Mar 2023 06:56:55 +0000 Gerrit-HasComments: Yes
