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

fix a bug about snapshot and adjust some test cases #213

Closed
wants to merge 3 commits into from

Conversation

hicqu
Copy link
Contributor

@hicqu hicqu commented Apr 10, 2019

The bug causes an exmaple panics in #205 .

In our implemetation, RaftLog::term(un_received_index) returns 0. So if a peer receives a snapshot or normal entry with log-term 0, RaftLog::match_term(log_term, un_received_index) will return true but it should be false exactly.

This PR fixes it by making RaftLog::match_term cover this case.

And, add a case test_snapshot_with_term_0 to cover it, and do some adjusts for some other test cases. At last, remove Interface::initial because we don't need it any more.

@hicqu
Copy link
Contributor Author

hicqu commented Apr 10, 2019

PTAL @Hoverbear @BusyJay thank you!

@@ -207,6 +211,15 @@ fn on_ready(
return;
}

// Apply the snashot. It's also necessary with same reason as above.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Apply the snashot. It's also necessary with same reason as above.
// Apply the snapshot. It's also necessary with same reason as above.

Also, which reason? That the program will be blocked forever?

@@ -61,21 +61,39 @@ pub struct Network {
}

impl Network {
/// Initializes a network from peers.
/// Get a base config. Calling `Network::new` will initialize peers with this config.
pub fn base_config() -> Config {
Copy link
Contributor

@Hoverbear Hoverbear Apr 10, 2019

Choose a reason for hiding this comment

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

Suggested change
pub fn base_config() -> Config {
pub fn default_config() -> Config {

@Hoverbear
Copy link
Contributor

Hoverbear commented Apr 10, 2019

@hicqu Seems maybe RaftLog::term(un_received_index) should return Option<u64> instead? Would that be a good way to fix this problem?

@hicqu
Copy link
Contributor Author

hicqu commented Apr 11, 2019

Thanks for your review @Hoverbear ! I will address them in #214 . Now I will close this PR because I prefer the another solution. :)

@hicqu hicqu closed this Apr 11, 2019
@bytewatch
Copy link

I think cause the root cause of the panic of five_mem_node is MemStorage::initialize_with_conf_state() :

        // To fix this panic,  add a more line: `core.raft_state.hard_state.set_commit(1)` , 
        // and handle the case that `prs.get_mut(self.id)` is None in restore_snapshot.
        // Or , just commet these two lines: 
        core.snapshot_metadata.set_index(1);  
        core.raft_state.hard_state.set_commit(1);  
        core.raft_state.conf_state = ConfState::from(conf_state);

self.term(idx).map(|t| t == term).unwrap_or(false)
match self.term(idx) {
// For uninitialized storage, should return false.
Ok(0) => term == 0 && self.last_index() > 0,

Choose a reason for hiding this comment

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

I don't think it need to add some logics that exceeds etcd-raft.

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.

3 participants