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 25: (10 comments) > Patch Set 24: > > (9 comments) http://gerrit.cloudera.org:8080/#/c/18572/23/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/18572/23/src/kudu/tools/kudu-admin-test.cc@3139 PS23, Line 3139: ASSERT_OK(sys_catalog.tablet_replica()->consensus()->WaitUntilLeader(kLeaderTimeout)); : TableInfoLoader table_info_loader; : sys_catalog.VisitTables(&table_info_loader); > move closer to the place where you use them. Done http://gerrit.cloudera.org:8080/#/c/18572/23/src/kudu/tools/kudu-admin-test.cc@3148 PS23, Line 3148: table_info = table; > meanless, remove it. Done http://gerrit.cloudera.org:8080/#/c/18572/23/src/kudu/tools/kudu-admin-test.cc@3160 PS23, Line 3160: tablets.push_back(tablet); > comments need update. Done http://gerrit.cloudera.org:8080/#/c/18572/23/src/kudu/tools/kudu-admin-test.cc@3162 PS23, Line 3162: } > Could you wrap these code to a function? Done http://gerrit.cloudera.org:8080/#/c/18572/23/src/kudu/tools/kudu-admin-test.cc@3165 PS23, Line 3165: bleMetadataLock l_table(table_info.get(), LockMode::WRITE); : TabletMetadataGroupLock l_tablets(LockMode::RELEASED); : l_t > assign directly? they are both std::vector<std::string> Done http://gerrit.cloudera.org:8080/#/c/18572/23/src/kudu/tools/master_rebuilder.cc File src/kudu/tools/master_rebuilder.cc: http://gerrit.cloudera.org:8080/#/c/18572/23/src/kudu/tools/master_rebuilder.cc@406 PS23, Line 406: return Status::Corruption("inconsistent replica: partition mismatch"); > Seems set is enough, no need map. Done http://gerrit.cloudera.org:8080/#/c/18572/23/src/kudu/tools/master_rebuilder.cc@431 PS23, Line 431: > warning: method 'DoWrite' can be made static [readability-convert-member-fu Done http://gerrit.cloudera.org:8080/#/c/18572/23/src/kudu/tools/master_rebuilder.cc@432 PS23, Line 432: > nit: how about table_by_name? Done http://gerrit.cloudera.org:8080/#/c/18572/23/src/kudu/tools/master_rebuilder.cc@433 PS23, Line 433: alog, tables_by_name_, > it's just an enum, const reference is no needed. Done http://gerrit.cloudera.org:8080/#/c/18572/23/src/kudu/tools/master_rebuilder.cc@434 PS23, Line 434: SysCatalogTable::SysCatalogOperation::ADD)); : return Status::OK(); > if map_tables is empty, you can skip it. 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: 25 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, 15 Jul 2022 11:24:11 +0000 Gerrit-HasComments: Yes
