fix a bug about initialized storage's term#214
Merged
hicqu merged 8 commits intotikv:masterfrom Apr 12, 2019
Merged
Conversation
Contributor
|
@hicqu Which solution do you think is most elegant and simple to understand? |
Hoverbear
reviewed
Apr 10, 2019
| 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 { |
Contributor
There was a problem hiding this comment.
Suggested change
| pub fn base_config() -> Config { | |
| pub fn default_config() -> Config { |
BusyJay
reviewed
Apr 11, 2019
| 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. |
Member
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
| /// 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() { |
| npeers.insert(id, r); | ||
| } | ||
| Some(mut p) => { | ||
| p.initial(id, &peer_addrs); |
Contributor
Author
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(); |
Member
There was a problem hiding this comment.
I don't think there will be any messages.
| (StateRole::Follower, 2, 2, INVALID_ID, true), | ||
| (StateRole::Follower, 2, 3, INVALID_ID, false), | ||
| (StateRole::Follower, 3, 0, INVALID_ID, true), | ||
| (StateRole::Follower, 3, 1, INVALID_ID, true), |
Member
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), |
Contributor
Author
|
@Hoverbear I think this PR is better because we won't meet |
Contributor
|
@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 <BusyJayLee@gmail.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.