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 18: (11 comments) http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/master/master.h File src/kudu/master/master.h: http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/master/master.h@102 PS18, Line 102: void close_fs_manager_for_test() { fs_manager_ = nullptr;} nit: better to define as private accessibility http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/master/sys_catalog.h File src/kudu/master/sys_catalog.h: http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/master/sys_catalog.h@104 PS18, Line 104: SimpleTableVisitor It's suggest to naming a more meaningful name. http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/tools/master_rebuilder.cc File src/kudu/tools/master_rebuilder.cc: http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/tools/master_rebuilder.cc@110 PS18, Line 110: strings nit: alignment http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/tools/master_rebuilder.cc@429 PS18, Line 429: = "" nit: can be omit. http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/tools/master_rebuilder.cc@428 PS18, Line 428: bool find_same_table_flag = false; : string old_table_name = ""; : string new_table_name = ""; emm, better to move closer to the place where it used. http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/tools/master_rebuilder.cc@430 PS18, Line 430: = "" nit: can be omit. http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/tools/master_rebuilder.cc@433 PS18, Line 433: for (const auto& table : visitor.tables) { You can define TableVisitor.tables as a associative or hash container, then you can reduce the time complex from O(n) to O(log(n)) or O(1). http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/tools/master_rebuilder.cc@437 PS18, Line 437: table_entry.second->metadata().ReadLock(); : new_table_name = table_entry.second->metadata().state().name(); : table_entry.second->metadata().ReadUnlock(); new_table_name can be obtained out of the loop, can't it? http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/tools/master_rebuilder.cc@446 PS18, Line 446: if (!find_same_table_flag) { find_same_table_flag can be removed if you do what I suggest above. http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/tools/master_rebuilder.cc@452 PS18, Line 452: const auto& table = table_entry.second; : table->GetAllTablets(&tablets); : TableMetadataLock l_table(table.get(), LockMode::WRITE); : TabletMetadataGroupLock l_tablets(LockMode::RELEASED); : l_tablets.AddMutableInfos(tablets); : l_tablets.Lock(LockMode::WRITE); : SysCatalogTable::Actions actions; : actions.table_to_update = table; : actions.tablets_to_update = tablets; : RETURN_NOT_OK_PREPEND(sys_catalog.Write(actions), : Substitute("unable to write metadata for table $0 to sys_catalog", : table_entry.first)); Could you please define a function for these code, and then you can reuse it in 3 places at least. http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/tools/tool_action_master.cc File src/kudu/tools/tool_action_master.cc: http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/tools/tool_action_master.cc@910 PS18, Line 910: create Some of the help info should be updated. -- 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: 18 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: Tue, 05 Jul 2022 15:50:11 +0000 Gerrit-HasComments: Yes
