Wang Xixu 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 20: (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: // Get the RPC and HTTP addresses for this master instance. > nit: better to define as private accessibility It can not be private, because it is used in outside. See kudu-admin-test.cc Line 3191 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: TableInfoVisitor : > It's suggest to naming a more meaningful name. Done 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: rings:: > nit: alignment Done http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/tools/master_rebuilder.cc@429 PS18, Line 429: bles_ > nit: can be omit. Done http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/tools/master_rebuilder.cc@428 PS18, Line 428: } else { : InsertOrDie(&tables_map_to_add, table_entry.first, table_entry.second); : } > emm, better to move closer to the place where it used. Done http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/tools/master_rebuilder.cc@430 PS18, Line 430: > nit: can be omit. Done http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/tools/master_rebuilder.cc@433 PS18, Line 433: TURN_NOT_OK(DoWrite(sys_catalog, tables_ma > You can define TableVisitor.tables as a associative or hash container, then Done http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/tools/master_rebuilder.cc@437 PS18, Line 437: : Status MasterRebuilder::DoWrite(SysCatalogTable sys_catalog, : const map<string, > new_table_name can be obtained out of the loop, can't it? Done http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/tools/master_rebuilder.cc@446 PS18, Line 446: TabletMetadataGroupLock l_tablets(LockMode::RELEASED); > find_same_table_flag can be removed if you do what I suggest above. Done http://gerrit.cloudera.org:8080/#/c/18572/18/src/kudu/tools/master_rebuilder.cc@452 PS18, Line 452: actions.table_to_add = table; : actions.tablets_to_add = tablets; : break; : } : case SysCatalogOperation::UPDATE: { : actions.table_to_update = table; : actions.tablets_to_update = tablets; : break; : } : case SysCatalogOperation::DELETE: { : actions.table_to_delete = table; : actions.tablets_to_delete = tablets; > Could you please define a function for these code, and then you can reuse i Done 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. Done -- 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: 20 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: Wed, 06 Jul 2022 11:14:27 +0000 Gerrit-HasComments: Yes
