Mike Percy has posted comments on this change. Change subject: Send back an error when UpdateConsensus cannot prepare a single transaction ......................................................................
Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/1785/2/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: Line 1132: if (deduped_req.messages.empty()) { I'm a little confused about this logic. Couldn't the leader have happened to have sent messages that we have already applied, perhaps due to a previous timeout? Also, I don't see a way that the body of this 'if' is reachable due to the check above where iter != deduped_req.messages.end() http://gerrit.cloudera.org:8080/#/c/1785/2/src/kudu/integration-tests/raft_consensus-itest.cc File src/kudu/integration-tests/raft_consensus-itest.cc: Line 2424: TEST_F(RaftConsensusITest, TestUpdateConsensusErrorNonePrepared) { I'd feel better about this test if it somehow tested that it indeed caused the leader to go into backoff mode when the problematic condition we saw on the cluster occurred. Line 2432: // Elect server 2 as leader. It's not necessary to elect someone here, you can just act as a leader directly. Line 2468: ASSERT_TRUE(resp.status().has_error()); Missing an assertion for the new error code -- To view, visit http://gerrit.cloudera.org:8080/1785 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I546fd3069af974383c23acb7406ea621e6962bb3 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
