Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: [master] KUDU-2181 Raft change config request for adding a master ...................................................................... Patch Set 3: (4 comments) Just a quick pass over + responding to comments http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/catalog_manager.cc@5564 PS2, Line 5564: auto comp_promise_ptr = s > ChangeConfig() implementation in Raft is inherently async, so earlier thoug See usages of RpcOpCompletionCallback in https://github.com/apache/kudu/blob/master/src/kudu/tserver/tablet_service.cc for how we do this on tservers for AlterSchema and Write requests. I don't mind making it synchronous. Might be worth passing in some RPC timeout from the RpcContext in MasterServiceImpl. http://gerrit.cloudera.org:8080/#/c/16321/3/src/kudu/master/master_options.h File src/kudu/master/master_options.h: http://gerrit.cloudera.org:8080/#/c/16321/3/src/kudu/master/master_options.h@51 PS3, Line 51: // Allows setting/overwriting list of masters. Only to be used by tests. : void SetMasterAddressesForTests(std::vector<HostPort> addresses) { : SetMasterAddresses(std::move(addresses)); : } What's the point of specifying this if it's the same as SetMasterAddresses()? Why not just make SetMasterAddresses() public? http://gerrit.cloudera.org:8080/#/c/16321/3/src/kudu/master/master_options.cc File src/kudu/master/master_options.cc: http://gerrit.cloudera.org:8080/#/c/16321/3/src/kudu/master/master_options.cc@110 PS3, Line 110: if (std::find(master_addresses_.begin(), master_addresses_.end(), hp) != : master_addresses_.end()) { : return Status::AlreadyPresent(Substitute("Master $0 already present", hp.ToString())); : } : : master_addresses_.emplace_back(hp); nit: this could be simplified with EmplaceIfNotPresent() from gutil/map-util.h http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master_service.cc@243 PS2, Line 243: if (!FLAGS_master_support_change_config) { : rpc->RespondFailure(Status::NotSupported("Adding master is not supported")); : return; : } > Thanks for the pointer. Added the feature in MasterFeatures as it helps wit Yeah, we should probably only add SupportsFeature() once the feature is complete. Until then, I'd be ok leaving this here. -- To view, visit http://gerrit.cloudera.org:8080/16321 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c Gerrit-Change-Number: 16321 Gerrit-PatchSet: 3 Gerrit-Owner: Bankim Bhavsar <[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: Sat, 12 Sep 2020 01:26:13 +0000 Gerrit-HasComments: Yes
