Todd Lipcon has submitted this change and it was merged. Change subject: KUDU-1735. Fix crash when aborting a skipped config change round ......................................................................
KUDU-1735. Fix crash when aborting a skipped config change round This fixes a crash seen in a test cluster when deleting a tablet in which there is a pending (uncommitted) REPLICATE message for a skipped CONFIG_CHANGE operation. The CONFIG_CHANGE was skipped because the tablet already had a higher indexed configuration written to disk. This can happen in a couple cases: one, exercised by a test included here, is if a tablet server crashes just after committing a config change to disk but before writing the COMMIT message to the WAL, then upon restart that config change will be a pending operation despite being committed. If the table is then deleted before the operation is committed, it would crash on abort. The approach taken to fix this is as follows: When aborting a config change operation, we only clear the 'pending' config if we see that its index matches the index of the operation being aborted. Otherwise, we ignore the abort (whereas we used to crash). In order to achieve this, we have to remember the index of the pending config, which previously wasn't set until after the commit. So, this assignment is moved out of the commit path and into AddPendingOperationUnlocked(). Changing the assumption of where the index was set involved making a few corresponding changes to DCHECKs elsewhere in the code. There is a slight wire/upgrade compatibility issue here: previously the leader would replicate config change messages with no opid set in the 'new_config'. Now, it does. I think this would cause DCHECKs to fire in a mixed-version cluster, though no CHECKs. Additionally, we aren't purporting to fully support rolling upgrade yet, so I don't think it's a big deal. I was able to upgrade the test cluster which experienced this issue in-place and it came up fine. This patch removes raft_consensus_state-test, which had some various assertions against setting pending configuration changes with opids set. After looking at this test, I realized that it's purporting to test ReplicaState but in fact all of the methods that it tests are just pass-throughs to ConsensusMeta and thus are already covered by consensus_meta-test. So, I figured there's no sense spending time updating this redundant test code. I ran raft_consensus-itest 1000 times[1]and the new test alone another 1000 times each in DEBUG[2] and TSAN[3] to test. [1] http://dist-test.cloudera.org/job?job_id=todd.1478141668.26073 [2] http://dist-test.cloudera.org/job?job_id=todd.1478291990.1887 [3] http://dist-test.cloudera.org/job?job_id=todd.1478292124.2771 Change-Id: I2b4e4eb1d60ef66527edf33357fabc22f4b5b32f Reviewed-on: http://gerrit.cloudera.org:8080/4916 Reviewed-by: Mike Percy <[email protected]> Tested-by: Kudu Jenkins --- M src/kudu/consensus/CMakeLists.txt M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/quorum_util.cc M src/kudu/consensus/raft_consensus.cc D src/kudu/consensus/raft_consensus_state-test.cc M src/kudu/consensus/raft_consensus_state.cc M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/raft_consensus-itest.cc 8 files changed, 119 insertions(+), 175 deletions(-) Approvals: Mike Percy: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/4916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I2b4e4eb1d60ef66527edf33357fabc22f4b5b32f Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]>
