Bankim Bhavsar 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: (3 comments) > Patch Set 3: > > > Patch Set 3: > > > > Build Started http://jenkins.kudu.apache.org/job/kudu-gerrit/22385/ > > One issue I realized is that --master_addressses for non-leader master > doesn't get updated in my changes so far. I need to think about how to solve > that. > > Maintaining separate in-memory state and the Raft config state for masters is > definitely tricky and I do recall pointing out in the design that getting rid > of the in-memory state might be an option instead of trying to keep them > in-sync. Updated the code such that master_addresses_ in MasterOptions is not updated on addition/removal of masters dynamically. MasterOptions is okay to use during master/catalog_manager init time but not after that. Updated comments in MasterOptions accordingly and mentioned using existing Master::GetMasterHostPorts() that fetches master addresses from local Raft config instead. This helps solve the problem with PS3 wherein only the current leader master had updated master addresses in MasterOptions. 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 > See usages of RpcOpCompletionCallback in https://github.com/apache/kudu/blo Thanks for the pointer. It's simple enough to pass the RpcContext to the completion callback to Raft which allows releasing the worker thread serving the RPC request sooner. So from the client point of view it's still synchronous but avoids blocking the RPC worker thread while the ChangeConfig request is being processed by the consensus engine. 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( Prevent callers from invoking SetMasterAddresses() if the function name clearly mentions "Tests". This is not needed anymore. 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-uti master_addresses_ is a vector, so couldn't use this. AddMaster() function is removed, so this is not needed. -- 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: Tue, 15 Sep 2020 21:55:13 +0000 Gerrit-HasComments: Yes
