Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

raftstore: handle conf change with snapshot correctly #6352

Merged
merged 18 commits into from Jan 10, 2020

Conversation

@hicqu
Copy link
Contributor

hicqu commented Dec 27, 2019

What have you changed?

Handle conf change with snapshot correctly. Close #6344 .

What is the type of the changes?

  • Bugfix (a change which fixes an issue)

How is the PR tested?

  • No code

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

No.

Does this PR affect tidb-ansible?

No.

Refer to a related PR or issue link (optional)

For some other fields in Raft which is changed on apply:
apply index: because Raft::restore(snap) won't change applied_index, so it's ok.

Benchmark result if necessary (optional)

Any examples? (optional)

@hicqu hicqu force-pushed the hicqu:handle-conf-change-error branch from d916abc to dd2074d Jan 2, 2020
src/raftstore/store/fsm/peer.rs Outdated Show resolved Hide resolved
tests/integrations/raftstore/test_conf_change.rs Outdated Show resolved Hide resolved
tests/integrations/raftstore/test_conf_change.rs Outdated Show resolved Hide resolved
tests/integrations/raftstore/test_conf_change.rs Outdated Show resolved Hide resolved
@hicqu hicqu added the S: WIP label Jan 3, 2020
Signed-off-by: qupeng <qupeng@pingcap.com>
@hicqu hicqu force-pushed the hicqu:handle-conf-change-error branch from 6103906 to d8fa856 Jan 7, 2020
@hicqu

This comment has been minimized.

Copy link
Contributor Author

hicqu commented Jan 7, 2020

/run-all-tests

1 similar comment
@hicqu

This comment has been minimized.

Copy link
Contributor Author

hicqu commented Jan 7, 2020

/run-all-tests

@hicqu hicqu removed the S: WIP label Jan 7, 2020
@hicqu

This comment has been minimized.

Copy link
Contributor Author

hicqu commented Jan 8, 2020

/run-all-tests

1 similar comment
@hicqu

This comment has been minimized.

Copy link
Contributor Author

hicqu commented Jan 8, 2020

/run-all-tests

@hicqu hicqu added this to the v4.0.0-beta milestone Jan 9, 2020
hicqu added 7 commits Jan 9, 2020
Signed-off-by: qupeng <qupeng@pingcap.com>
@hicqu

This comment has been minimized.

Copy link
Contributor Author

hicqu commented Jan 10, 2020

/run-all-tests

1 similar comment
@hicqu

This comment has been minimized.

Copy link
Contributor Author

hicqu commented Jan 10, 2020

/run-all-tests

src/raftstore/store/fsm/apply.rs Outdated Show resolved Hide resolved
src/raftstore/store/fsm/peer.rs Outdated Show resolved Hide resolved
src/raftstore/store/fsm/peer.rs Outdated Show resolved Hide resolved
tests/Cargo.toml Outdated Show resolved Hide resolved
tests/failpoints/cases/test_conf_change.rs Outdated Show resolved Hide resolved
hicqu added 4 commits Jan 10, 2020
Signed-off-by: qupeng <qupeng@pingcap.com>
Signed-off-by: qupeng <qupeng@pingcap.com>
cluster.sim.wl().add_send_filter(3, filter);

cluster.must_transfer_leader(1, new_peer(3, 3));
assert!(rx.recv_timeout(Duration::from_secs(3)).is_err());

This comment has been minimized.

Copy link
@BusyJay

BusyJay Jan 10, 2020

Contributor

Try recv is enough. If peer (4, 4) is in memory, a request vote must have been sent already.

.when(Arc::new(AtomicBool::new(false)))
.set_msg_callback(cb),
);
cluster.sim.wl().add_send_filter(3, filter);

This comment has been minimized.

Copy link
@BusyJay

BusyJay Jan 10, 2020

Contributor

Better add the filter before L264.

This comment has been minimized.

Copy link
@hicqu

hicqu Jan 10, 2020

Author Contributor

Why? I think only need to be before transferring leadership to peer 3.


// Clear filters on peer 3, so it can receive and restore a snapshot.
cluster.sim.wl().clear_recv_filters(3);
sleep_ms(100);

This comment has been minimized.

Copy link
@BusyJay

BusyJay Jan 10, 2020

Contributor

Why sleep?

This comment has been minimized.

Copy link
@hicqu

hicqu Jan 10, 2020

Author Contributor

Without sleep the snapshot may come after peer 3 calling apply_conf_change.

hicqu added 4 commits Jan 10, 2020
Signed-off-by: qupeng <qupeng@pingcap.com>
Signed-off-by: qupeng <qupeng@pingcap.com>
Signed-off-by: qupeng <qupeng@pingcap.com>
@hicqu hicqu changed the title raftstore: handle conf change error correctly raftstore: fix a consistency bug about conf change in snapshot Jan 10, 2020
@hicqu hicqu changed the title raftstore: fix a consistency bug about conf change in snapshot raftstore: handle conf change with snapshot correctly Jan 10, 2020
Copy link
Contributor

overvenus left a comment

RestLGTM

_ => unreachable!(),
}
} else {
// Please take a look at test case test_redundant_conf_change_by_snapshot.

This comment has been minimized.

Copy link
@overvenus

overvenus Jan 10, 2020

Contributor

Could you move comments here?

@AndreMouche AndreMouche merged commit 6153b19 into tikv:master Jan 10, 2020
3 checks passed
3 checks passed
DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.