You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/09/14 00:22:03 UTC
[kudu-CR] WIP: fix consensus divergence issue
Hello David Ribeiro Alves,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4409
to review the following change.
Change subject: WIP: fix consensus divergence issue
......................................................................
WIP: fix consensus divergence issue
Change-Id: I2fb95b447991b7cadc2c403bc2596fead0bd31ad
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/consensus_queue.h
M src/kudu/consensus/log_cache.cc
M src/kudu/consensus/log_cache.h
M src/kudu/consensus/raft_consensus-test.cc
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_state.cc
M src/kudu/consensus/raft_consensus_state.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_verifier.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
A src/kudu/integration-tests/log_verifier.cc
A src/kudu/integration-tests/log_verifier.h
14 files changed, 269 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/4409/1
--
To view, visit http://gerrit.cloudera.org:8080/4409
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2fb95b447991b7cadc2c403bc2596fead0bd31ad
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
[kudu-CR] consensus: properly truncate all state when aborting operations
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: consensus: properly truncate all state when aborting operations
......................................................................
consensus: properly truncate all state when aborting operations
This fixes a consensus bug which was causing exactly_once_writes-itest
to be slightly flaky. The issue was the following sequence:
- a node A is a follower, and has some operations appended (eg 10.5 through
10.7)
- a node B is elected for term 11, and sends node 'A' a status-only request
containing preceding_op_id=11.6
-- node 'A' aborts operations 10.6 and 10.7
-- HOWEVER: it was not explicitly removing these operations from the
LogCache or the Log. Removal was only happening on an actual
operation _replacement_.
- node 'B' loses its leadership before it is able to replicate anything
to a majority
- node 'A' gets elected for term 12
-- it calls Queue::SetLeaderMode()
-- this triggers the first requests to be sent to the peer
-- we hit a race where the first request is being constructed _before_
the leader appends its initial NO_OP to the queue
--- because we never truncated the log cache or queue, we see operations
10.6 and 10.7 in the queue, and send them to a follower
-- we now append the NO_OP 12.6 which replaces the aborted 10.6.
In this case, the peer who received the fauly request from the leader
may end up committing those operations whereas the rest of the nodes
commit operations from term 12.
The fix in this patch is to explicitly truncate the queue and the
LogCache state when we are aborting operations. WIP because it needs a
few more comments.
To test, I looped exactly_once_writes-itest --gtest_filter=\*Churny\*
1000 times before and after.
Without the patch[1], I got 17 failures, 16 of which were verification
errors that one of the committed op terms did not match.
With the patch[2], I got 5 failures, all of which were checksum
errors while verifying the logs. Since seeing those failures, I fixed
the verifier to run only after shutting down the cluster.
[1] http://dist-test.cloudera.org/job?job_id=todd.1473812577.12216
[2] http://dist-test.cloudera.org/job?job_id=todd.1473811112.9830
Change-Id: I2fb95b447991b7cadc2c403bc2596fead0bd31ad
Reviewed-on: http://gerrit.cloudera.org:8080/4409
Tested-by: Kudu Jenkins
Reviewed-by: David Ribeiro Alves <dr...@apache.org>
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/consensus_queue.h
M src/kudu/consensus/log_cache-test.cc
M src/kudu/consensus/log_cache.cc
M src/kudu/consensus/log_cache.h
M src/kudu/consensus/raft_consensus-test.cc
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_state.cc
M src/kudu/consensus/raft_consensus_state.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_verifier.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
A src/kudu/integration-tests/log_verifier.cc
A src/kudu/integration-tests/log_verifier.h
15 files changed, 351 insertions(+), 22 deletions(-)
Approvals:
David Ribeiro Alves: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4409
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I2fb95b447991b7cadc2c403bc2596fead0bd31ad
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] consensus: properly truncate all state when aborting operations
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4409
to look at the new patch set (#5).
Change subject: consensus: properly truncate all state when aborting operations
......................................................................
consensus: properly truncate all state when aborting operations
This fixes a consensus bug which was causing exactly_once_writes-itest
to be slightly flaky. The issue was the following sequence:
- a node A is a follower, and has some operations appended (eg 10.5 through
10.7)
- a node B is elected for term 11, and sends node 'A' a status-only request
containing preceding_op_id=11.6
-- node 'A' aborts operations 10.6 and 10.7
-- HOWEVER: it was not explicitly removing these operations from the
LogCache or the Log. Removal was only happening on an actual
operation _replacement_.
- node 'B' loses its leadership before it is able to replicate anything
to a majority
- node 'A' gets elected for term 12
-- it calls Queue::SetLeaderMode()
-- this triggers the first requests to be sent to the peer
-- we hit a race where the first request is being constructed _before_
the leader appends its initial NO_OP to the queue
--- because we never truncated the log cache or queue, we see operations
10.6 and 10.7 in the queue, and send them to a follower
-- we now append the NO_OP 12.6 which replaces the aborted 10.6.
In this case, the peer who received the fauly request from the leader
may end up committing those operations whereas the rest of the nodes
commit operations from term 12.
The fix in this patch is to explicitly truncate the queue and the
LogCache state when we are aborting operations. WIP because it needs a
few more comments.
To test, I looped exactly_once_writes-itest --gtest_filter=\*Churny\*
1000 times before and after.
Without the patch[1], I got 17 failures, 16 of which were verification
errors that one of the committed op terms did not match.
With the patch[2], I got 5 failures, all of which were checksum
errors while verifying the logs. Since seeing those failures, I fixed
the verifier to run only after shutting down the cluster.
[1] http://dist-test.cloudera.org/job?job_id=todd.1473812577.12216
[2] http://dist-test.cloudera.org/job?job_id=todd.1473811112.9830
Change-Id: I2fb95b447991b7cadc2c403bc2596fead0bd31ad
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/consensus_queue.h
M src/kudu/consensus/log_cache-test.cc
M src/kudu/consensus/log_cache.cc
M src/kudu/consensus/log_cache.h
M src/kudu/consensus/raft_consensus-test.cc
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_state.cc
M src/kudu/consensus/raft_consensus_state.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_verifier.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
A src/kudu/integration-tests/log_verifier.cc
A src/kudu/integration-tests/log_verifier.h
15 files changed, 351 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/4409/5
--
To view, visit http://gerrit.cloudera.org:8080/4409
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2fb95b447991b7cadc2c403bc2596fead0bd31ad
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] consensus: properly truncate all state when aborting operations
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4409
to look at the new patch set (#4).
Change subject: consensus: properly truncate all state when aborting operations
......................................................................
consensus: properly truncate all state when aborting operations
This fixes a consensus bug which was causing exactly_once_writes-itest
to be slightly flaky. The issue was the following sequence:
- a node A is a follower, and has some operations appended (eg 10.5 through
10.7)
- a node B is elected for term 11, and sends node 'A' a status-only request
containing preceding_op_id=11.6
-- node 'A' aborts operations 10.6 and 10.7
-- HOWEVER: it was not explicitly removing these operations from the
LogCache or the Log. Removal was only happening on an actual
operation _replacement_.
- node 'B' loses its leadership before it is able to replicate anything
to a majority
- node 'A' gets elected for term 12
-- it calls Queue::SetLeaderMode()
-- this triggers the first requests to be sent to the peer
-- we hit a race where the first request is being constructed _before_
the leader appends its initial NO_OP to the queue
--- because we never truncated the log cache or queue, we see operations
10.6 and 10.7 in the queue, and send them to a follower
-- we now append the NO_OP 12.6 which replaces the aborted 10.6.
In this case, the peer who received the fauly request from the leader
may end up committing those operations whereas the rest of the nodes
commit operations from term 12.
The fix in this patch is to explicitly truncate the queue and the
LogCache state when we are aborting operations. WIP because it needs a
few more comments.
To test, I looped exactly_once_writes-itest --gtest_filter=\*Churny\*
1000 times before and after.
Without the patch[1], I got 17 failures, 16 of which were verification
errors that one of the committed op terms did not match.
With the patch[2], I got 5 failures, all of which were checksum
errors while verifying the logs. Since seeing those failures, I fixed
the verifier to run only after shutting down the cluster.
[1] http://dist-test.cloudera.org/job?job_id=todd.1473812577.12216
[2] http://dist-test.cloudera.org/job?job_id=todd.1473811112.9830
Change-Id: I2fb95b447991b7cadc2c403bc2596fead0bd31ad
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/consensus_queue.h
M src/kudu/consensus/log_cache-test.cc
M src/kudu/consensus/log_cache.cc
M src/kudu/consensus/log_cache.h
M src/kudu/consensus/raft_consensus-test.cc
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_state.cc
M src/kudu/consensus/raft_consensus_state.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_verifier.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
A src/kudu/integration-tests/log_verifier.cc
A src/kudu/integration-tests/log_verifier.h
15 files changed, 346 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/4409/4
--
To view, visit http://gerrit.cloudera.org:8080/4409
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2fb95b447991b7cadc2c403bc2596fead0bd31ad
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] consensus: properly truncate all state when aborting operations
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4409
to look at the new patch set (#3).
Change subject: consensus: properly truncate all state when aborting operations
......................................................................
consensus: properly truncate all state when aborting operations
This fixes a consensus bug which was causing exactly_once_writes-itest
to be slightly flaky. The issue was the following sequence:
- a node A is a follower, and has some operations appended (eg 10.5 through
10.7)
- a node B is elected for term 11, and sends node 'A' a status-only request
containing preceding_op_id=11.6
-- node 'A' aborts operations 10.6 and 10.7
-- HOWEVER: it was not explicitly removing these operations from the
LogCache or the Log. Removal was only happening on an actual
operation _replacement_.
- node 'B' loses its leadership before it is able to replicate anything
to a majority
- node 'A' gets elected for term 12
-- it calls Queue::SetLeaderMode()
-- this triggers the first requests to be sent to the peer
-- we hit a race where the first request is being constructed _before_
the leader appends its initial NO_OP to the queue
--- because we never truncated the log cache or queue, we see operations
10.6 and 10.7 in the queue, and send them to a follower
-- we now append the NO_OP 12.6 which replaces the aborted 10.6.
In this case, the peer who received the fauly request from the leader
may end up committing those operations whereas the rest of the nodes
commit operations from term 12.
The fix in this patch is to explicitly truncate the queue and the
LogCache state when we are aborting operations. WIP because it needs a
few more comments.
To test, I looped exactly_once_writes-itest --gtest_filter=\*Churny\*
1000 times before and after.
Without the patch[1], I got 17 failures, 16 of which were verification
errors that one of the committed op terms did not match.
With the patch[2], I got 5 failures, all of which were checksum
errors while verifying the logs. Since seeing those failures, I fixed
the verifier to run only after shutting down the cluster.
[1] http://dist-test.cloudera.org/job?job_id=todd.1473812577.12216
[2] http://dist-test.cloudera.org/job?job_id=todd.1473811112.9830
Change-Id: I2fb95b447991b7cadc2c403bc2596fead0bd31ad
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/consensus_queue.h
M src/kudu/consensus/log_cache-test.cc
M src/kudu/consensus/log_cache.cc
M src/kudu/consensus/log_cache.h
M src/kudu/consensus/raft_consensus-test.cc
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_state.cc
M src/kudu/consensus/raft_consensus_state.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_verifier.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
A src/kudu/integration-tests/log_verifier.cc
A src/kudu/integration-tests/log_verifier.h
15 files changed, 339 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/4409/3
--
To view, visit http://gerrit.cloudera.org:8080/4409
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2fb95b447991b7cadc2c403bc2596fead0bd31ad
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] consensus: properly truncate all state when aborting operations
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: consensus: properly truncate all state when aborting operations
......................................................................
Patch Set 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/4409/3/src/kudu/consensus/consensus_queue.h
File src/kudu/consensus/consensus_queue.h:
Line 184: virtual void TruncateOpsAfter(const OpId& op);
> docs
Done
http://gerrit.cloudera.org:8080/#/c/4409/3/src/kudu/consensus/log_cache.h
File src/kudu/consensus/log_cache.h:
Line 103: // HasOpBeenWritten(), etc, will return as if the operations were never appended.
> should we mention that this is not across restarts?
Done
http://gerrit.cloudera.org:8080/#/c/4409/3/src/kudu/integration-tests/log_verifier.cc
File src/kudu/integration-tests/log_verifier.cc:
Line 28: #include "kudu/util/metrics.h"
> order
Done
Line 38: using strings::Substitute;
> order
Done
Line 115: all_op_indexes.insert(index_term.first);
> can we also test that we don't skip indexes somewhere?
skipping COMMIT indexes should be allowed because of out-of-order commits in conjunction with log GC, right?
Line 125: committed_terms.push_back(FindWithDefault(maps_by_ts[ts], index, -1));
> use a constant that maybe helps explain what this is doing?
Done. Also switched to using boost::optional for the expected_term
Line 153:
> nit extra line
Done
--
To view, visit http://gerrit.cloudera.org:8080/4409
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2fb95b447991b7cadc2c403bc2596fead0bd31ad
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: properly truncate all state when aborting operations
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: consensus: properly truncate all state when aborting operations
......................................................................
Patch Set 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/4409/3/src/kudu/consensus/consensus_queue.h
File src/kudu/consensus/consensus_queue.h:
Line 184: virtual void TruncateOpsAfter(const OpId& op);
docs
http://gerrit.cloudera.org:8080/#/c/4409/3/src/kudu/consensus/log_cache.h
File src/kudu/consensus/log_cache.h:
Line 103: // HasOpBeenWritten(), etc, will return as if the operations were never appended.
should we mention that this is not across restarts?
http://gerrit.cloudera.org:8080/#/c/4409/3/src/kudu/integration-tests/log_verifier.cc
File src/kudu/integration-tests/log_verifier.cc:
Line 28: #include "kudu/util/metrics.h"
order
Line 38: using strings::Substitute;
order
Line 115: all_op_indexes.insert(index_term.first);
can we also test that we don't skip indexes somewhere?
Line 125: committed_terms.push_back(FindWithDefault(maps_by_ts[ts], index, -1));
use a constant that maybe helps explain what this is doing?
Line 153:
nit extra line
--
To view, visit http://gerrit.cloudera.org:8080/4409
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2fb95b447991b7cadc2c403bc2596fead0bd31ad
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: properly truncate all state when aborting operations
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: consensus: properly truncate all state when aborting operations
......................................................................
Patch Set 3:
Build Started http://104.196.14.100/job/kudu-gerrit/3428/
--
To view, visit http://gerrit.cloudera.org:8080/4409
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2fb95b447991b7cadc2c403bc2596fead0bd31ad
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] consensus: properly truncate all state when aborting operations
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: consensus: properly truncate all state when aborting operations
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4409
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2fb95b447991b7cadc2c403bc2596fead0bd31ad
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] consensus: properly truncate all state when aborting operations
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: consensus: properly truncate all state when aborting operations
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4409/4/src/kudu/consensus/raft_consensus_state.h
File src/kudu/consensus/raft_consensus_state.h:
Line 235: void AbortOpsAfterUnlocked(int64_t index);
> warning: function 'kudu::consensus::ReplicaState::AbortOpsAfterUnlocked' ha
Done
--
To view, visit http://gerrit.cloudera.org:8080/4409
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2fb95b447991b7cadc2c403bc2596fead0bd31ad
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] WIP: fix consensus divergence issue
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: WIP: fix consensus divergence issue
......................................................................
Patch Set 1:
Build Started http://104.196.14.100/job/kudu-gerrit/3411/
--
To view, visit http://gerrit.cloudera.org:8080/4409
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2fb95b447991b7cadc2c403bc2596fead0bd31ad
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] consensus: properly truncate all state when aborting operations
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4409
to look at the new patch set (#2).
Change subject: consensus: properly truncate all state when aborting operations
......................................................................
consensus: properly truncate all state when aborting operations
This fixes a consensus bug which was causing exactly_once_writes-itest
to be slightly flaky. The issue was the following sequence:
- a node A is a follower, and has some operations appended (eg 10.5 through
10.7)
- a node B is elected for term 11, and sends node 'A' a status-only request
containing preceding_op_id=11.6
-- node 'A' aborts operations 10.6 and 10.7
-- HOWEVER: it was not explicitly removing these operations from the
LogCache or the Log. Removal was only happening on an actual
operation _replacement_.
- node 'B' loses its leadership before it is able to replicate anything
to a majority
- node 'A' gets elected for term 12
-- it calls Queue::SetLeaderMode()
-- this triggers the first requests to be sent to the peer
-- we hit a race where the first request is being constructed _before_
the leader appends its initial NO_OP to the queue
--- because we never truncated the log cache or queue, we see operations
10.6 and 10.7 in the queue, and send them to a follower
-- we now append the NO_OP 12.6 which replaces the aborted 10.6.
In this case, the peer who received the fauly request from the leader
may end up committing those operations whereas the rest of the nodes
commit operations from term 12.
The fix in this patch is to explicitly truncate the queue and the
LogCache state when we are aborting operations. WIP because it needs a
few more comments.
To test, I looped exactly_once_writes-itest --gtest_filter=\*Churny\*
1000 times before and after.
Without the patch[1], I got 17 failures, 16 of which were verification
errors that one of the committed op terms did not match.
With the patch[2], I got 5 failures, all of which were checksum
errors while verifying the logs. Since seeing those failures, I fixed
the verifier to run only after shutting down the cluster.
[1] http://dist-test.cloudera.org/job?job_id=todd.1473812577.12216
[2] http://dist-test.cloudera.org/job?job_id=todd.1473811112.9830
Change-Id: I2fb95b447991b7cadc2c403bc2596fead0bd31ad
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/consensus_queue.h
M src/kudu/consensus/log_cache.cc
M src/kudu/consensus/log_cache.h
M src/kudu/consensus/raft_consensus-test.cc
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_state.cc
M src/kudu/consensus/raft_consensus_state.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_verifier.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
A src/kudu/integration-tests/log_verifier.cc
A src/kudu/integration-tests/log_verifier.h
14 files changed, 272 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/4409/2
--
To view, visit http://gerrit.cloudera.org:8080/4409
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2fb95b447991b7cadc2c403bc2596fead0bd31ad
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] consensus: properly truncate all state when aborting operations
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: consensus: properly truncate all state when aborting operations
......................................................................
Patch Set 2:
Build Started http://104.196.14.100/job/kudu-gerrit/3413/
--
To view, visit http://gerrit.cloudera.org:8080/4409
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2fb95b447991b7cadc2c403bc2596fead0bd31ad
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] consensus: properly truncate all state when aborting operations
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: consensus: properly truncate all state when aborting operations
......................................................................
Patch Set 3:
oops, I purposefully broke the test case that failed in order to test that my matcher was working :) please review, will put up a new rev that un-breaks the test case soon.
--
To view, visit http://gerrit.cloudera.org:8080/4409
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2fb95b447991b7cadc2c403bc2596fead0bd31ad
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No