Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16936 )
Change subject: [master] KUDU-2181 Raft ChangeConfig request to remove a master ...................................................................... Patch Set 2: Code-Review+1 (5 comments) http://gerrit.cloudera.org:8080/#/c/16936/1/src/kudu/master/dynamic_multi_master-test.cc File src/kudu/master/dynamic_multi_master-test.cc: http://gerrit.cloudera.org:8080/#/c/16936/1/src/kudu/master/dynamic_multi_master-test.cc@805 PS1, Line 805: ASSERT_OK(cluster_->GetLeaderMasterIndex(&leader_master_idx)); > I did hit following error during development of the test and now that I rec Ah, would running something like WaitForCatalogManager() help? If not, leaving this here sgtm. http://gerrit.cloudera.org:8080/#/c/16936/1/src/kudu/master/dynamic_multi_master-test.cc@853 PS1, Line 853: > ExternalMiniCluster interface doesn't allow updating the masters or more sp Ack http://gerrit.cloudera.org:8080/#/c/16936/2/src/kudu/master/dynamic_multi_master-test.cc File src/kudu/master/dynamic_multi_master-test.cc: http://gerrit.cloudera.org:8080/#/c/16936/2/src/kudu/master/dynamic_multi_master-test.cc@779 PS2, Line 779: // Tests removing a non-leader master from the cluster. As a followup, would it make sense, after removing the master, to delete the contents of the old master's directory and run through the 'add' steps as if it were a new master? Or maybe once the tooling is complete, as an end-to-end test. http://gerrit.cloudera.org:8080/#/c/16936/2/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/16936/2/src/kudu/master/master.cc@526 PS2, Line 526: for (const auto& peer : config.peers()) { We had a group discussion about whether to use UUIDs vs hostports. Summarizing the output, I think the consensus was that it made sense to use the hostports by default, allow an optional UUID, and only use the UUID if there are multiple candidates with the same hostports. Is that an apt summary? If so, is that something you intend on addressing in this patch, or in a follow-up (i'm fine with either)? http://gerrit.cloudera.org:8080/#/c/16936/2/src/kudu/mini-cluster/external_mini_cluster.h File src/kudu/mini-cluster/external_mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/16936/2/src/kudu/mini-cluster/external_mini_cluster.h@483 PS2, Line 483: // Removes the master from the ExternalMiniCluster when the master has been removed : // externally by the test after bringing up the ExternalMiniCluster. This helps : // keep the state of the actual cluster in sync with the state in ExternalMiniCluster. nit: IMO this is still a little confusing, given it's very easy to conflate what "remove" means in this sentence. How about: "Removes any bookkeeping about the given master UUID in the ExternalMiniCluster, _after_ already having run through a successful Master Raft change config to remove it." -- To view, visit http://gerrit.cloudera.org:8080/16936 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I76c03b8850faef60b65f85184c0a4db7cc6759ee Gerrit-Change-Number: 16936 Gerrit-PatchSet: 2 Gerrit-Owner: Bankim Bhavsar <[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-Comment-Date: Sat, 30 Jan 2021 00:03:14 +0000 Gerrit-HasComments: Yes
