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 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: string stdout; : string stderr; : const string& part_tables = Substitute("$0,$1", kTable1, kTable2); move closer to the place where you use them. http://gerrit.cloudera.org:8080/#/c/18572/23/src/kudu/tools/kudu-admin-test.cc@3148 PS23, Line 3148: ASSERT_STR_NOT_CONTAINS(stderr, "must be empty"); meanless, remove it. http://gerrit.cloudera.org:8080/#/c/18572/23/src/kudu/tools/kudu-admin-test.cc@3160 PS23, Line 3160: Rebuild 1 table in add mode. comments need update. 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? http://gerrit.cloudera.org:8080/#/c/18572/23/src/kudu/tools/kudu-admin-test.cc@3165 PS23, Line 3165: for (const string& data_dir : cluster_->master()->data_dirs()) { : opts.fs_opts.data_roots.push_back(data_dir); : } assign directly? they are both std::vector<std::string> 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: } Seems set is enough, no need map. http://gerrit.cloudera.org:8080/#/c/18572/23/src/kudu/tools/master_rebuilder.cc@432 PS23, Line 432: nit: how about table_by_name? http://gerrit.cloudera.org:8080/#/c/18572/23/src/kudu/tools/master_rebuilder.cc@433 PS23, Line 433: ogTable::SysCatalogOperati it's just an enum, const reference is no needed. http://gerrit.cloudera.org:8080/#/c/18572/23/src/kudu/tools/master_rebuilder.cc@434 PS23, Line 434: return Status::OK(); : } if map_tables is empty, you can skip it. -- 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: 24 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 10:12:55 +0000 Gerrit-HasComments: Yes
