fix a bug about initialized storage's term #214
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@hicqu Which solution do you think is most elegant and simple to understand? |
Hoverbear
reviewed
Apr 10, 2019
harness/src/network.rs
Outdated
| @@ -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 { | |||
There was a problem hiding this comment.
Suggested change
| pub fn base_config() -> Config { | |
| pub fn default_config() -> Config { |
Hoverbear
requested review from
Hoverbear and
BusyJay
and removed request for
Hoverbear
BusyJay
reviewed
Apr 11, 2019
src/raft.rs
Outdated
| @@ -2040,7 +2044,10 @@ impl<T: Storage> Raft<T> { | |||
|
|
|||
| let next_idx = self.raft_log.last_index() + 1; | |||
| let mut prs = ProgressSet::restore_snapmeta(meta, next_idx, self.max_inflight); | |||
| prs.get_mut(self.id).unwrap().matched = next_idx - 1; | |||
| if let Some(pr) = prs.get_mut(self.id) { | |||
| // The snapshot could be too old so that it doesn't contain the peer. | |||
There was a problem hiding this comment.
Why? How can a leader send a snapshot to a node that it doesn't know?
BusyJay
reviewed
Apr 11, 2019
harness/src/network.rs
Outdated
| /// Initialize a network from `peers` with explicitly specified `config`. | ||
| pub fn new_with_config(mut peers: Vec<Option<Interface>>, config: &Config) -> Network { | ||
| let peer_addrs: Vec<u64> = (1..=peers.len() as u64).collect(); | ||
| for (i, id) in peer_addrs.iter().enumerate() { |
| let r = Raft::new(&config, store).unwrap().into(); | ||
| npeers.insert(id, r); | ||
| } | ||
| Some(mut p) => { | ||
| p.initial(id, &peer_addrs); |
There was a problem hiding this comment.
Because all tests which need initial are fixed. I think initial is a not a good logic because it does additional changes to the peers, which should be already set correctly when the peer is created by Raft::new.
| let n2 = new_test_raft_with_prevote(2, vec![], 10, 1, new_storage(), pre_vote); | ||
| let mut nt = Network::new(vec![Some(n1), Some(n2)]); | ||
| nt.send(vec![new_message(1, 1, MessageType::MsgHup, 0)]); | ||
| let messages = nt.read_messages(); |
There was a problem hiding this comment.
I don't think there will be any messages.
| (StateRole::Follower, 2, 3, INVALID_ID, false), | ||
| (StateRole::Follower, 3, 0, INVALID_ID, true), | ||
| (StateRole::Follower, 3, 1, INVALID_ID, true), |
There was a problem hiding this comment.
What's the difference between L1425 and L1426?
| (StateRole::Leader, 3, 3, 1, true), | ||
| (StateRole::PreCandidate, 3, 3, 1, true), | ||
| (StateRole::Candidate, 3, 3, 1, true), | ||
| (StateRole::Follower, 4, 1, INVALID_ID, true), |
|
@Hoverbear I think this PR is better because we won't meet |
|
@hicqu Ok, do you want to close the other then? |
BusyJay
approved these changes
Apr 12, 2019
nolouch
approved these changes
Apr 12, 2019
BusyJay
added a commit
that referenced
this pull request
Mar 26, 2020
This PR removes the modifications of index during initialization, which will allow raft's log index goes from 0 again. This is important as many existing test cases rely on it, especially those related to flow control. Those cases pass on master in an accidental way, checking what are exact broken will be too much work. Besides upstream patches from etcd also rely on the same assumption, only forbid growing from 0 in raft-rs will make it hard to port patches. Most of the changes are basically revert of index changes introduced from #196 and #214. Signed-off-by: Jay Lee <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Try to resolve same problem with #213 , but with a different way.
This PR changes initialized storage's term from
0to1. So all place we need to callmatch_termwon't get0as parameter, so the problem is resolved.However more test cases need to be updated.
Please compare this PR with #213 , we only need to merge 1.