Dinesh Bhat has posted comments on this change. Change subject: KUDU-1330: Add a tool to unsafely recover from loss of majority replicas ......................................................................
Patch Set 11: (12 comments) TFTR Mike for continued efforts in reviewing this patch, addressed all comments below after discussions offline, please take a look. 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. > Still not seeing this check in Rev 12 I wanted more clarity here offline, so updated now. Line 1601: new_peer.last_known_addr().host() == committed_peer.last_known_addr().host() && > I am confused about this comment. We already have them in the committed con Done Line 1623: RaftConfigPB new_config = config; > My only concern is that users have to specify all this stuff like ports whe updated, after the discussion offline. http://gerrit.cloudera.org:8080/#/c/6066/12/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: PS12, Line 1597: new_peer : co > unused in this patch added the missing code, and also updated comments here. PS12, Line 1652: 64 replicate_opid_index = preceding_opid.index() + 1; : consensus_req.set_cal > Can we remove this part of the comment? Done http://gerrit.cloudera.org:8080/#/c/6066/11/src/kudu/consensus/raft_consensus_state.cc File src/kudu/consensus/raft_consensus_state.cc: PS11, Line 248: pending_config.allow_unsafe() && config_to_commit.allow_unsafe() > I think the code in rev 12 is correct. I am a little confused by this comme If we know for sure that config_to_commit(with unsafe flag set) does have a pending config which was never overwritten, then we can take that config_to_commit through equality check. For eg, in your example unsafe config operations above, the last "remote_replica unsafe_change_config a.b.c.d foo A" operation can still go through this equality check validation and succeed. 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/uns Done PS12, Line 238: In the event of unsafe config change enforced from an external tool > In the event of an unsafe config change triggered by an administrator, Done PS12, Line 239: config > the config Done PS12, Line 240: tool overwrote that pending config with 'unsafe' flag. The new config however : // has the 'unsafe' flag set and has a pending config too. So only the config : // which has the 'unsafe' flag set both in p > How about: Done 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 > This test is to ensure that we can handle multiple pending config changes, Done 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, > Why don't we just allow the user to specify UUIDs only? We can do that sinc done, reverted all of these changes. -- 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 <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
