Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18572 )
Change subject: [Tools] Rebuild master according to part of tables not all tables ...................................................................... Patch Set 21: (11 comments) http://gerrit.cloudera.org:8080/#/c/18572/21/src/kudu/master/sys_catalog.h File src/kudu/master/sys_catalog.h: http://gerrit.cloudera.org:8080/#/c/18572/21/src/kudu/master/sys_catalog.h@104 PS21, Line 104: class TableInfoVisitor : public TableVisitor { Seems similar to class TestTableLoader in src/kudu/master/sys_catalog-test.cc, could you do some refactor to reuse it? http://gerrit.cloudera.org:8080/#/c/18572/21/src/kudu/master/sys_catalog.h@415 PS21, Line 415: TableInfoVisitor table_info_loader_; I don't think it's necessary to make it as a member, isn't it? http://gerrit.cloudera.org:8080/#/c/18572/21/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: http://gerrit.cloudera.org:8080/#/c/18572/21/src/kudu/master/sys_catalog.cc@1165 PS21, Line 1165: const SysTablesEntryPB& metadata) { nit: alignment http://gerrit.cloudera.org:8080/#/c/18572/21/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/18572/21/src/kudu/tools/kudu-admin-test.cc@3143 PS21, Line 3143: /*is_secure*/false, /*log_to_stderr*/true, part_tables), nit: alignment http://gerrit.cloudera.org:8080/#/c/18572/21/src/kudu/tools/kudu-admin-test.cc@3157 PS21, Line 3157: Rebuild 3 tables in add mode. Have you miss this test case? http://gerrit.cloudera.org:8080/#/c/18572/21/src/kudu/tools/kudu-admin-test.cc@3177 PS21, Line 3177: foo1 Should be 'default.foo1'? And it's better to use kTable1 instead. http://gerrit.cloudera.org:8080/#/c/18572/21/src/kudu/tools/kudu-admin-test.cc@3187 PS21, Line 3187: } should break? http://gerrit.cloudera.org:8080/#/c/18572/21/src/kudu/tools/kudu-admin-test.cc@3191 PS21, Line 3191: NO_FATALS(master.close_fs_manager_for_test()); You can destroy 'master' then you don't have to close_fs_manager_for_test manually. Or you can close fs_manager in master::Shutdown(). http://gerrit.cloudera.org:8080/#/c/18572/21/src/kudu/tools/master_rebuilder.cc File src/kudu/tools/master_rebuilder.cc: http://gerrit.cloudera.org:8080/#/c/18572/21/src/kudu/tools/master_rebuilder.cc@411 PS21, Line 411: tables_map_to_update Naming it as <value>_by_<key> is a good practice in Kudu, e.g. std::unordered_map<string, TabletSizeStats> size_stats_by_table_id in src/kudu/tools/tool_action_local_replica.cc, it's suggest to rename these variables here. http://gerrit.cloudera.org:8080/#/c/18572/21/src/kudu/tools/master_rebuilder.cc@425 PS21, Line 425: map_tables_in_sys_catalog.find(new_table_name) != : map_tables_in_sys_catalog.end() Could you plz use ContainsKey in src/kudu/gutil/map-util.h to simplify code? http://gerrit.cloudera.org:8080/#/c/18572/21/src/kudu/tools/tool_action_master.cc File src/kudu/tools/tool_action_master.cc: http://gerrit.cloudera.org:8080/#/c/18572/21/src/kudu/tools/tool_action_master.cc@931 PS21, Line 931: "- The parameter 'tables' can be used to rebuild part of tables, when these tables " Add a blank space before '-' -- To view, visit http://gerrit.cloudera.org:8080/18572 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I45d812c8a16228ac937603872161969eb05136eb Gerrit-Change-Number: 18572 Gerrit-PatchSet: 21 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Fri, 08 Jul 2022 07:30:42 +0000 Gerrit-HasComments: Yes
