Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: WIP [master]: Initiate change config request for adding master ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/consensus/raft_consensus.cc@2467 PS1, Line 2467: const auto& consensus = cmeta_->ActiveConfig(); : auto member_type = RaftPeerPB::UNKNOWN_MEMBER_TYPE; : const auto& local_peer_uuid = peer_uuid(); > Can we do any of these outside the lock? ActiveConfig() returns a const reference. So getting access to the reference using const ptr under lock and then accessing the consensus state wouldn't be thread safe. The thread safe option is creating of copy under the lock and then using the local copy to access members of consensus without lock. However RaftConsensusPB looks like a fairly large object so copy won't be cheap. So considering lock is held for iterating over small number of peers(typically 3) and the copy won't be cheap, holding lock looks reasonable. Moved couple of variable declarations above the lock. http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/master/catalog_manager.cc@5491 PS1, Line 5491: if (!submission_status.ok()) { : // Caller will log the submission_status error, this is for the additional error_code. : VLOG(1) << Substitute("Failed initiating ChangeConfig request to $0 master, error: $1", : add_remove_str, : error_code ? TabletServerErrorPB::Code_Name(*error_code) : "unknown"); : } > Rather than logging, how about passing back the error code via RETURN_NOT_O Done http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/master/dynamic_multi_master-test.cc File src/kudu/master/dynamic_multi_master-test.cc: http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/master/dynamic_multi_master-test.cc@79 PS1, Line 79: opts.bind_mode = BindMode::LOOPBACK; > Why not just leave the default? Done http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/master/dynamic_multi_master-test.cc@186 PS1, Line 186: } : }); > To be sure the config change is persisted, how about restarting the cluster Done -- 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: 1 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 02 Sep 2020 17:24:42 +0000 Gerrit-HasComments: Yes
