Mike Percy has posted comments on this change. Change subject: KUDU-1330: Add a tool to unsafely recover from loss of majority replicas ......................................................................
Patch Set 11: (7 comments) http://gerrit.cloudera.org:8080/#/c/6066/11/src/kudu/consensus/consensus.proto File src/kudu/consensus/consensus.proto: PS11, Line 522: Unsafe > I initially started with that naming convention, but changed it to Unsafe o I guess Force is ambiguous. Manual seems a little too specific to me. I'm okay with leaving this as Unsafe http://gerrit.cloudera.org:8080/#/c/6066/11/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: Line 1592: // Check that passed replica uuids are part of the committed config on this node. > Done, I didn't use that routine because we already can compare directly its Still not seeing this check in Rev 12 Line 1601: new_peer.last_known_addr().host() == committed_peer.last_known_addr().host() && > yeah we do. If you can recall, there was a reason why we chose RaftConfigPB I am confused about this comment. We already have them in the committed config. Why not just use it from there? Line 1623: RaftConfigPB new_config = config; > That's a good idea, but given that we are supplying the entire config to th My only concern is that users have to specify all this stuff like ports when they don't need to. http://gerrit.cloudera.org:8080/#/c/6066/12/src/kudu/consensus/raft_consensus_state.cc File src/kudu/consensus/raft_consensus_state.cc: PS12, Line 212: config Based on the latest discussion in the review comments let's do s/forced/unsafe/ http://gerrit.cloudera.org:8080/#/c/6066/11/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: PS11, Line 665: fault > Sure, just to be sure this has nothing todo with pending config right ? Can This test is to ensure that we can handle multiple pending config changes, since A will not be able to commit any of those successive unsafe config changes until the last one comes in, since the rest of the cluster is down. At the end, A should commit the last config change (with only itself in the cluster) and the last committed log index should have incremented by as many configs as we appended to its log ( plus one for the election no-op). http://gerrit.cloudera.org:8080/#/c/6066/11/src/kudu/tools/tool_action_common.h File src/kudu/tools/tool_action_common.h: Line 95: Status ParseHostPortString(const std::string& hostport_str, > No, but IIRC I believe not allowing default port was intentional. We are us Why don't we just allow the user to specify UUIDs only? We can do that since currently we only allow them to remove nodes from the config. -- To view, visit http://gerrit.cloudera.org:8080/6066 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I908d8c981df74d56dbd034e72001d379fb314700 Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
