Alexey Serbin 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 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/16936/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16936/1//COMMIT_MSG@12 PS1, Line 12: tests I'm curious what's the behavior of Kudu client when a master is removed underneath after the client has connected to the cluster. Does it make sense to add such tests for both C++ and Java clients? 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@254 PS1, Line 254: cluster nit: it would be great to document the 'cluster' parameter as well in the comment above http://gerrit.cloudera.org:8080/#/c/16936/1/src/kudu/master/dynamic_multi_master-test.cc@938 PS1, Line 938: non_leader_master_idx Does it matter whether it's leader or non-leader master index? I guess if the functionality is controlled by feature flags, this should matter since the feature flags verification happens before any application-specific code is invoked. As such, I guess here we should check both masters and see the same error, right? http://gerrit.cloudera.org:8080/#/c/16936/1/src/kudu/master/dynamic_multi_master-test.cc@995 PS1, Line 995: It's possible there is a leadership change between querying for leader master and : // sending the add master request to non-leader master and hence using ASSERT_EVENTUALLY. > Is this not a concern in other tests? Would setting --leader_failure_max_mi In tablet server tests there is another way to protect against this -- disable master elections and then issue an election request manually. This might be an option here as well, I guess? http://gerrit.cloudera.org:8080/#/c/16936/1/src/kudu/master/dynamic_multi_master-test.cc@1009 PS1, Line 1009: ASSERT_OK(cluster_->master_proxy(non_leader_master_idx)->RemoveMaster(req, &resp, &rpc)); > What if this succeeds because we happened to elect non_leader_master_idx as Yep, I'm curious whether this might end up in removing one master, and then trying to remove the only one which is left? http://gerrit.cloudera.org:8080/#/c/16936/1/src/kudu/master/dynamic_multi_master-test.cc@1019 PS1, Line 1019: TEST_F(DynamicMultiMasterTest, TestRemoveLeaderMaster) { Does it make sense to add a simple scenario to verify that a single and the only master cannot be removed from a cluster? http://gerrit.cloudera.org:8080/#/c/16936/1/src/kudu/master/master.h File src/kudu/master/master.h: http://gerrit.cloudera.org:8080/#/c/16936/1/src/kudu/master/master.h@131 PS1, Line 131: of nit: of initiating ? http://gerrit.cloudera.org:8080/#/c/16936/1/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/16936/1/src/kudu/master/master.cc@523 PS1, Line 523: if (HostPortFromPB(peer.last_known_addr()) == hp Could there be more than one master satisfying this criterion? If so, what should be the behavior -- just remove the first matching one? http://gerrit.cloudera.org:8080/#/c/16936/1/src/kudu/master/master.cc@532 PS1, Line 532: leader master nit: I guess it might be not only the leader, right? Maybe, just report that a master cannot remove itself from the config? http://gerrit.cloudera.org:8080/#/c/16936/1/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/16936/1/src/kudu/master/master.proto@941 PS1, Line 941: HostPortPB Why HostPostPB instead of UUID as an identifier of existing server is better in this context? -- 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: 1 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 12 Jan 2021 03:50:02 +0000 Gerrit-HasComments: Yes
