Andrew Wong 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: (5 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? 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_OK_PREPEND? 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? http://gerrit.cloudera.org:8080/#/c/16321/1/src/kudu/master/dynamic_multi_master-test.cc@173 PS1, Line 173: // since the system catalog is essentially empty. Perhaps consider creating a table at least so we can verify the table gets replicated upon catching the new master up. With so little in the WAL, we shouldn't have to worry about GC here. It'd also be good to verify that after adding the master, if we create a table, the new master's catalog also gets updated, or better yet, force the new master to become the leader. 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 after this and trying to list the masters again? -- 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: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 11 Aug 2020 17:59:21 +0000 Gerrit-HasComments: Yes
