Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17528 )
Change subject: [master] AddMaster and copy sys catalog automatically ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/17528/3/src/kudu/master/master_runner.cc File src/kudu/master/master_runner.cc: http://gerrit.cloudera.org:8080/#/c/17528/3/src/kudu/master/master_runner.cc@145 PS3, Line 145: Returns true if all the above criteria are met, in which case 'leader_hp' is : // populated with the current leader's hostport. Otherwise, returns false, : // indicating callers should try again. Could you clarify the purpose of "local_hp" and "has_errors" as out parameters? What does it mean for the callers when Status is OK but has_errors is false? http://gerrit.cloudera.org:8080/#/c/17528/3/src/kudu/master/master_runner.cc@197 PS3, Line 197: const auto& cstate = resp.tablets(0).cstate(); It'd be good to supply the system catalog tablet id in the request instead of trying to fetch all in case of new tablets in masters in future. http://gerrit.cloudera.org:8080/#/c/17528/3/src/kudu/master/master_runner.cc@230 PS3, Line 230: auto& const auto& http://gerrit.cloudera.org:8080/#/c/17528/3/src/kudu/master/master_runner.cc@233 PS3, Line 233: LOG(INFO) << "Remote masters have differing Raft configurations..."; Might be worth printing the symmetric set difference to help debug. http://gerrit.cloudera.org:8080/#/c/17528/3/src/kudu/master/master_runner.cc@327 PS3, Line 327: if (current_master_uuids.size() + 1 != opts.master_addresses().size()) { : return Status::NotSupported("Kudu only supports adding one master at a time"); : } In addition to checking for size, lets also check that opts.master_addresses only contains itself the one extra master that needs to be added. Basically we don't want a case where this local master has multiple changes like addition/removal combination. http://gerrit.cloudera.org:8080/#/c/17528/3/src/kudu/master/master_runner.cc@334 PS3, Line 334: Could the part below of adding master/deleting catalog/copying catalog move to a separate function/file? http://gerrit.cloudera.org:8080/#/c/17528/3/src/kudu/master/master_runner.cc@371 PS3, Line 371: RETURN_NOT_OK(TSTabletManager::DeleteTabletData( It'd be good to add a log line mentioning the local sys catalog is being deleted. -- To view, visit http://gerrit.cloudera.org:8080/17528 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7635723f30317a45799ad7b9c9b725eafbd735b Gerrit-Change-Number: 17528 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 06 Jul 2021 22:33:00 +0000 Gerrit-HasComments: Yes
