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

Check pending conf change before campaign #225

Merged
merged 4 commits into from
Apr 29, 2019

Conversation

BusyJay
Copy link
Member

@BusyJay BusyJay commented Apr 28, 2019

Fix #221.

This is a fix for 0.4.x, I will do the fix for master later.

@CLAassistant
Copy link

CLAassistant commented Apr 28, 2019

CLA assistant check
All committers have signed the CLA.

@siddontang
Copy link
Contributor

@nolouch

I think we should contribute to etcd later.

@siddontang
Copy link
Contributor

PTAL @xiang90


/// Tests if unapplied conf change is checked before campaign.
#[test]
fn test_conf_change_check_before_campaign() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we don't call self.hub(true), the test can fail?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The case was written before the fix, and it failed.

@BusyJay
Copy link
Member Author

BusyJay commented Apr 28, 2019

Seems criterion's API is broken. @Hoverbear Would you like to take a look?

@siddontang
Copy link
Contributor

@BusyJay

seem some failed tests were not caused by criterion API changes.

@BusyJay
Copy link
Member Author

BusyJay commented Apr 29, 2019

@siddontang can you name some? I can't find them.

@siddontang
Copy link
Contributor

thread 'raft_log::test::test_commit_to' panicked at ' to_commit 4 is out of range [last_index 3]', src/raft_log.rs:248:13
test raft_log::test::test_append ... ok
thread 'raft_log::test::test_compaction' panicked at 'compact 1001 is out of bound lastindex(1000)', src/storage.rs:174:13
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:39
   1: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:70
   2: std::panicking::default_hook::{{closure}}
             at src/libstd/sys_common/backtrace.rs:58
             at src/libstd/panicking.rs:200
   3: std::panicking::default_hook
             at src/libstd/panicking.rs:215
   4: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:478
   5: std::panicking::continue_panic_fmt
             at src/libstd/panicking.rs:385
   6: std::panicking::begin_panic_fmt
             at src/libstd/panicking.rs:340
   7: <raft::raft_log::RaftLog<T>>::commit_to
             at src/raft_log.rs:248
   8: raft::raft_log::test::test_commit_to::{{closure}}
             at src/raft_log.rs:1285
   9: core::ops::function::FnOnce::call_once
             at /rustc/fc50f328b0353b285421b8ff5d4100966387a997/src/libcore/ops/function.rs:231
  10: <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
             at /rustc/fc50f328b0353b285421b8ff5d4100966387a997/src/libstd/panic.rs:309
  11: std::panicking::try::do_call
             at /rustc/fc50f328b0353b285421b8ff5d4100966387a997/src/libstd/panicking.rs:297
  12: __rust_maybe_catch_panic
             at src/libpanic_unwind/lib.rs:87
  13: std::panicking::try
             at /rustc/fc50f328b0353b285421b8ff5d4100966387a997/src/libstd/panicking.rs:276
  14: std::panic::catch_unwind
             at /rustc/fc50f328b0353b285421b8ff5d4100966387a997/src/libstd/panic.rs:388
  15: raft::raft_log::test::test_commit_to
             at src/raft_log.rs:1285
  16: raft::raft_log::test::test_commit_to::{{closure}}
             at src/raft_log.rs:1270
  17: core::ops::function::FnOnce::call_once
             at /rustc/fc50f328b0353b285421b8ff5d4100966387a997/src/libcore/ops/function.rs:231
  18: <F as alloc::boxed::FnBox<A>>::call_box
             at src/libtest/lib.rs:1497
             at /rustc/fc50f328b0353b285421b8ff5d4100966387a997/src/libcore/ops/function.rs:231
             at /rustc/fc50f328b0353b285421b8ff5d4100966387a997/src/liballoc/boxed.rs:749
  19: __rust_maybe_catch_panic
             at src/libpanic_unwind/lib.rs:87
  20: test::run_test::run_test_inner::{{closure}}
             at /rustc/fc50f328b0353b285421b8ff5d4100966387a997/src/libstd/panicking.rs:276
             at /rustc/fc50f328b0353b285421b8ff5d4100966387a997/src/libstd/panic.rs:388
             at src/libtest/lib.rs:1452

@BusyJay
Copy link
Member Author

BusyJay commented Apr 29, 2019

Those tests are expected to panic. They are wrapped with UnwindSafe.

src/raft.rs Outdated
);
});
let n = self.num_pending_conf(&ents);
if n != 0 && self.raft_log.committed > self.raft_log.applied {
Copy link
Contributor

@hicqu hicqu Apr 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a pending snapshot, what will raft_log.applied be?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case I think raft_log.applied must be less than snapshot.index, so I think only n != 0 is ok here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

raft_log.applied can be any value less than the snapshot index.

@hicqu
Copy link
Contributor

hicqu commented Apr 29, 2019

LGTM.

@BusyJay BusyJay merged commit 64571f0 into tikv:0.4.x Apr 29, 2019
@BusyJay BusyJay deleted the safe-transfer-leader branch April 29, 2019 08:05
hicqu pushed a commit to hicqu/raft-rs that referenced this pull request Jul 17, 2019
Hoverbear pushed a commit that referenced this pull request Jul 19, 2019
* Check pending conf change before campaign (#225)

Fix #221.

* Add more convenient lite-weight interfaces (#227)

This PR introduces two simple and lite weight interfaces:
- ping to trigger heartbeats without ticking,
- status_ref to borrow the progress set instead of cloning.

* *: bump to 0.4.2 (#228)

* Bump to v0.4.3 (#231)

* raft: leader respond to learner read index message (#220)

Signed-off-by: nolouch <nolouch@gmail.com>

* Bump to v0.4.3

Signed-off-by: Neil Shen <overvenus@gmail.com>

* Request snapshot (#243)

Signed-off-by: Neil Shen <overvenus@gmail.com>

* fix tests

* cargo fmt

* address comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants