make configuration change effective on received by hicqu · Pull Request #202 · tikv/raft-rs

hicqu · 2019-04-02T07:41:47Z

Make configuration changes effective on received.

Some test cases about membership change are removed:

tests in test_membership_change::api.
Because now these API are removed.
pending_delete_fails_after_begin, pending_create_with_quorum_fails_after_begin, pending_create_and_destroy_both_fail.
They are covered by minority_followers_halt.

Hoverbear · 2019-04-03T00:14:23Z

@hicqu Can you make the CI pass?

hicqu · 2019-04-03T06:26:39Z

PTAL @BusyJay @Hoverbear @nolouch thank you!

src/progress/progress_set.rs

src/raft.rs

src/raft_log.rs

src/storage.rs

src/progress/progress_set.rs

harness/src/network.rs

hicqu · 2019-04-09T05:13:48Z

PTAL again, thank you. @Hoverbear @BusyJay

src/raft.rs

Hoverbear · 2019-04-11T19:45:29Z

@hicqu I'm sorry about the merge conflicts, we merged #201. :(

Could you fix the merge conflicts? I think in most cases it should be straightforward. You can ask @nrc or @ice1000 if you encounter any strange problems.

ice1000 · 2019-04-11T19:50:30Z

Replace ::new() with ::new_()
Replace use protobuf::Message with use prost::Message as Msg
Replace x.compute_size() with Msg::encoded_len(&x) (where this Msg is prost::Message)
Replace RepeatedField::from_vec(x) with x
Replace let data = protobuf::Message::write_to_bytes(&cc)?; with let mut data = Vec::with_capacity(ProstMsg::encoded_len(&cc)); cc.encode(&mut data)?;

hicqu · 2019-04-12T11:01:55Z

@Hoverbear it doesn't matter. I was waiting for #214 exactly. Could you take a look again? Thanks!

Hoverbear · 2019-04-12T19:42:54Z

@hicqu You have a clippy lint fail :(

The command "if [[ $TRAVIS_RUST_VERSION == "stable" && $TRAVIS_OS_NAME == "linux" ]]; then cargo fmt -- --check; fi" exited with 0.
9.10s$ if [[ $TRAVIS_RUST_VERSION == "stable" && $TRAVIS_OS_NAME == "linux" ]]; then cargo clippy -- -D clippy::all; fi
    Updating crates.io index
   Compiling raft v0.5.0 (/home/travis/build/pingcap/raft-rs)
error: redundant closure found
   --> src/progress/progress_set.rs:673:49
    |
673 |         self.next_configuration = next_conf.map(|c| c.into());
    |                                                 ^^^^^^^^^^^^ help: remove closure as shown: `std::convert::Into::into`
    |
note: lint level defined here
   --> src/lib.rs:360:9
    |
360 | #![deny(clippy::all)]
    |         ^^^^^^^^^^^
    = note: #[deny(clippy::redundant_closure)] implied by #[deny(clippy::all)]
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure
error: aborting due to previous error
error: Could not compile `raft`.
To learn more, run the command again with --verbose.
The command "if [[ $TRAVIS_RUST_VERSION == "stable" && $TRAVIS_OS_NAME == "linux" ]]; then cargo clippy -- -D clippy::all; fi" exited with 101.

harness/src/network.rs

src/prost/wrapper_eraftpb.rs

src/raft.rs

tests/integration_cases/test_membership_changes.rs

Signed-off-by: qupeng <qupeng@pingcap.com>

BusyJay · 2019-10-10T10:06:50Z

harness/tests/integration_cases/test_raw_node.rs

-                let mut conf_change = ConfChange::default();
-                conf_change.merge_from_bytes(&e.data).unwrap();
-                raw_node.apply_conf_change(&conf_change).ok();
+        if raw_node.propose_conf_change(vec![], cc).is_ok() {


Why not unwrap directly?

harness/tests/integration_cases/test_raw_node.rs

BusyJay · 2019-10-10T10:11:59Z

harness/tests/integration_cases/test_raw_node.rs

    assert_eq!(nt.peers[&1].raft_log.committed, 7);
    assert_eq!(nt.peers[&2].raft_log.committed, 7);
-    assert_eq!(nt.peers[&3].raft_log.committed, 7);
+    assert_eq!(nt.peers[&3].raft_log.committed, 5);


This shows a problem that a node will not get notified that it's removed.

Signed-off-by: qupeng <qupeng@pingcap.com>

BusyJay · 2019-10-11T02:20:53Z

Bugs described in https://groups.google.com/forum/#!msg/raft-dev/t4xj6dJTP6E/d2D9LrWRza8J is not addressed in this PR.

fix a bug about single node membership change Signed-off-by: qupeng <qupeng@pingcap.com>

fix all test cases Signed-off-by: qupeng <qupeng@pingcap.com>

Signed-off-by: qupeng <qupeng@pingcap.com>

BusyJay · 2019-10-14T07:11:21Z

src/progress/progress_set.rs

    next_configuration: Option<Configuration>,

+    /// Uncommitted removed progresses with indices, which must be monotonically increasing.
+    pub(crate) uncommitted_removes: Vec<(u64, Vec<u64>)>,


GC after committed is simpler.

Signed-off-by: qupeng <qupeng@pingcap.com>

harness/tests/integration_cases/test_raft.rs

Signed-off-by: qupeng <qupeng@pingcap.com>

harness/tests/integration_cases/test_raft.rs

src/progress/progress_set.rs

Signed-off-by: qupeng <qupeng@pingcap.com>

BusyJay · 2019-10-17T07:04:58Z

src/progress/progress_set.rs

-        assert!(
-            check_membership_change_configuration((vec![1], vec![]), (vec![], vec![]),).is_err()
-        )
+        assert!(check_membership_change_configuration((vec![1], vec![]), (vec![], vec![]),).is_ok())


/cc @Hoverbear PTAL

It's because currently an empty configuration is allowed as target. Do you think we need to ban it? @Hoverbear

I'm not really sure it's a valid state.

I guess it's valid means there is a machanism to remove all peers from a Raft group, although TiKV doesn't need it.

Signed-off-by: qupeng <qupeng@pingcap.com>

BusyJay · 2019-10-18T08:33:33Z

src/storage.rs

            if snap.get_metadata().index < request_index {
+                // NOTE: the logic is only for tests.
                snap.mut_metadata().index = request_index;
+                snap.mut_metadata().conf_state_index = request_index;


This is confusing. What if there are conf change between conf_state_index and request_index?

BusyJay · 2019-10-18T08:46:04Z

src/raw_node.rs

            ..Default::default()
        };
+        if let Some(e) = rd.entries.first() {
+            for i in (0..raft.conf_states().len()).rev() {


Why not use Iterator::find? It seems more understandable.

Seems it's better to find it from tail to head because there could be many unapplied entries in conf_states.

You mean entries.iter().rev().position()?

BusyJay · 2019-10-18T08:46:20Z

src/raw_node.rs

+                if raft.conf_states()[i].index >= e.index {
+                    continue;
+                }
+                if i < raft.conf_states().len() {


I think it's always true.

Signed-off-by: qupeng <qupeng@pingcap.com>

hicqu · 2019-10-29T04:34:15Z

Close. Need to find other solutions for etcd-io/etcd#11284.

hicqu requested review from BusyJay and Hoverbear April 2, 2019 08:19