From bda8acced136bf5b86df8820c3a0521929bc8a32 Mon Sep 17 00:00:00 2001 From: Hoverbear Date: Wed, 8 Aug 2018 11:27:13 -0700 Subject: [PATCH 1/5] Use impl trait in progress --- src/progress.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/progress.rs b/src/progress.rs index 3e19e595b..1ed39b000 100644 --- a/src/progress.rs +++ b/src/progress.rs @@ -27,8 +27,7 @@ use fxhash::FxHashMap; use std::cmp; -use std::collections::hash_map::{HashMap, Iter, IterMut}; -use std::iter::Chain; +use std::collections::hash_map::HashMap; /// The state of the progress. #[derive(Debug, PartialEq, Clone, Copy)] @@ -105,12 +104,12 @@ impl ProgressSet { } /// Returns an iterator across all the nodes and their progress. - pub fn iter(&self) -> Chain, Iter> { + pub fn iter(&self) -> impl Iterator { self.voters.iter().chain(&self.learners) } /// Returns a mutable iterator across all the nodes and their progress. - pub fn iter_mut(&mut self) -> Chain, IterMut> { + pub fn iter_mut(&mut self) -> impl Iterator { self.voters.iter_mut().chain(&mut self.learners) } From 1676ccd24a653259d6aee2b6d4877a725f074730 Mon Sep 17 00:00:00 2001 From: Hoverbear Date: Wed, 8 Aug 2018 11:36:09 -0700 Subject: [PATCH 2/5] Make ProgressSet not panic. Moves failable functions to returning `Result<(), Error>` and adds useful error types for determining the problem. Adds an `panic` to all calling code to preserve existing behavior. --- src/errors.rs | 9 ++++++- src/progress.rs | 53 ++++++++++++++++++++-------------------- src/raft.rs | 20 +++++++++++---- tests/cases/test_raft.rs | 8 ++++-- 4 files changed, 55 insertions(+), 35 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index 0b5935c69..c76c44696 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -55,7 +55,14 @@ quick_error! { description(err.description()) display("protobuf error {:?}", err) } - + /// The node exists, but should not. + Exists(id: u64, set: &'static str) { + display("The node {} aleady exists in the {} set.", id, set) + } + /// The node does not exist, but should. + NotExists(id: u64, set: &'static str) { + display("The node {} is not in the {} set.", id, set) + } } } diff --git a/src/progress.rs b/src/progress.rs index 1ed39b000..478cc0747 100644 --- a/src/progress.rs +++ b/src/progress.rs @@ -28,6 +28,7 @@ use fxhash::FxHashMap; use std::cmp; use std::collections::hash_map::HashMap; +use errors::Error; /// The state of the progress. #[derive(Debug, PartialEq, Clone, Copy)] @@ -114,31 +115,27 @@ impl ProgressSet { } /// Adds a voter node - /// - /// # Panics - /// - /// Panics if the node already has been added. - pub fn insert_voter(&mut self, id: u64, pr: Progress) { - if self.learners.contains_key(&id) { - panic!("insert voter {} but already in learners", id); + pub fn insert_voter(&mut self, id: u64, pr: Progress) -> Result<(), Error> { + if self.voters.contains_key(&id) { + Err(Error::Exists(id, "voters"))? } - if self.voters.insert(id, pr).is_some() { - panic!("insert voter {} twice", id); + if self.learners.contains_key(&id) { + Err(Error::Exists(id, "learners"))?; } + self.voters.insert(id, pr); + Ok(()) } /// Adds a learner to the cluster - /// - /// # Panics - /// - /// Panics if the node already has been added. - pub fn insert_learner(&mut self, id: u64, pr: Progress) { + pub fn insert_learner(&mut self, id: u64, pr: Progress) -> Result<(), Error> { if self.voters.contains_key(&id) { - panic!("insert learner {} but already in voters", id); + Err(Error::Exists(id, "voters"))? } - if self.learners.insert(id, pr).is_some() { - panic!("insert learner {} twice", id); + if self.learners.contains_key(&id) { + Err(Error::Exists(id, "learners"))? } + self.learners.insert(id, pr); + Ok(()) } /// Removes the peer from the set of voters or learners. @@ -150,17 +147,19 @@ impl ProgressSet { } /// Promote a learner to a peer. - /// - /// # Panics - /// - /// Panics if the node doesn't exist. - pub fn promote_learner(&mut self, id: u64) { - if let Some(mut pr) = self.learners.remove(&id) { - pr.is_learner = false; - self.voters.insert(id, pr); - return; + pub fn promote_learner(&mut self, id: u64) -> Result<(), Error> { + if self.voters.contains_key(&id) { + Err(Error::Exists(id, "voters"))?; + } + if !self.learners.contains_key(&id) { + Err(Error::NotExists(id, "learners"))? } - panic!("promote not exists learner: {}", id); + + self.learners.remove(&id).map(|mut learner| { + learner.is_learner = false; + self.voters.insert(id, learner); + }); + Ok(()) } } diff --git a/src/raft.rs b/src/raft.rs index dd2eec435..a2232f988 100644 --- a/src/raft.rs +++ b/src/raft.rs @@ -263,12 +263,16 @@ impl Raft { }; for p in peers { let pr = new_progress(1, r.max_inflight); - r.mut_prs().insert_voter(*p, pr); + if let Err(e) = r.mut_prs().insert_voter(*p, pr) { + panic!("{}", e); + } } for p in learners { let mut pr = new_progress(1, r.max_inflight); pr.is_learner = true; - r.mut_prs().insert_learner(*p, pr); + if let Err(e) = r.mut_prs().insert_learner(*p, pr) { + panic!("{}", e); + }; if *p == r.id { r.is_learner = true; } @@ -1852,7 +1856,9 @@ impl Raft { // Ignore redundant add learner. return; } - self.mut_prs().promote_learner(id); + if let Err(e) = self.mut_prs().promote_learner(id) { + panic!("{}", e) + } if id == self.id { self.is_learner = false; } @@ -1903,9 +1909,13 @@ impl Raft { p.matched = matched; p.is_learner = is_learner; if is_learner { - self.mut_prs().insert_learner(id, p); + if let Err(e) = self.mut_prs().insert_learner(id, p) { + panic!("{}", e); + } } else { - self.mut_prs().insert_voter(id, p); + if let Err(e) = self.mut_prs().insert_voter(id, p) { + panic!("{}", e); + } } } diff --git a/tests/cases/test_raft.rs b/tests/cases/test_raft.rs index 66bef9500..291e0c368 100644 --- a/tests/cases/test_raft.rs +++ b/tests/cases/test_raft.rs @@ -209,12 +209,16 @@ impl Interface { is_learner: true, ..Default::default() }; - self.mut_prs().insert_learner(*id, progress); + if let Err(e) = self.mut_prs().insert_learner(*id, progress) { + panic!("{}", e); + } } else { let progress = Progress { ..Default::default() }; - self.mut_prs().insert_voter(*id, progress); + if let Err(e) = self.mut_prs().insert_voter(*id, progress) { + panic!("{}", e); + } } } let term = self.term; From 50d80cb67f7d120d9a5493c29d94ed6a3eb76226 Mon Sep 17 00:00:00 2001 From: Hoverbear Date: Wed, 8 Aug 2018 14:11:04 -0700 Subject: [PATCH 3/5] clippy/fmt run --- src/progress.rs | 11 +++++------ src/raft.rs | 6 ++---- src/storage.rs | 2 +- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/progress.rs b/src/progress.rs index 478cc0747..04b059a16 100644 --- a/src/progress.rs +++ b/src/progress.rs @@ -25,10 +25,10 @@ // See the License for the specific language governing permissions and // limitations under the License. +use errors::Error; use fxhash::FxHashMap; use std::cmp; use std::collections::hash_map::HashMap; -use errors::Error; /// The state of the progress. #[derive(Debug, PartialEq, Clone, Copy)] @@ -105,12 +105,12 @@ impl ProgressSet { } /// Returns an iterator across all the nodes and their progress. - pub fn iter(&self) -> impl Iterator { + pub fn iter(&self) -> impl Iterator { self.voters.iter().chain(&self.learners) } /// Returns a mutable iterator across all the nodes and their progress. - pub fn iter_mut(&mut self) -> impl Iterator { + pub fn iter_mut(&mut self) -> impl Iterator { self.voters.iter_mut().chain(&mut self.learners) } @@ -154,11 +154,10 @@ impl ProgressSet { if !self.learners.contains_key(&id) { Err(Error::NotExists(id, "learners"))? } - - self.learners.remove(&id).map(|mut learner| { + if let Some(mut learner) = self.learners.remove(&id) { learner.is_learner = false; self.voters.insert(id, learner); - }); + } Ok(()) } } diff --git a/src/raft.rs b/src/raft.rs index a2232f988..4f4f62823 100644 --- a/src/raft.rs +++ b/src/raft.rs @@ -1912,10 +1912,8 @@ impl Raft { if let Err(e) = self.mut_prs().insert_learner(id, p) { panic!("{}", e); } - } else { - if let Err(e) = self.mut_prs().insert_voter(id, p) { - panic!("{}", e); - } + } else if let Err(e) = self.mut_prs().insert_voter(id, p) { + panic!("{}", e); } } diff --git a/src/storage.rs b/src/storage.rs index 9c6aecffc..a8809ef9c 100644 --- a/src/storage.rs +++ b/src/storage.rs @@ -200,7 +200,7 @@ impl MemStorageCore { // truncate compacted entries let te: &[Entry] = if first > ents[0].get_index() { let start_ent = (first - ents[0].get_index()) as usize; - &ents[start_ent..ents.len()] + &ents[start_ent..] } else { ents }; From fca5a21e7a577eb35d0c46df1582a7973d848d1e Mon Sep 17 00:00:00 2001 From: Hoverbear Date: Tue, 14 Aug 2018 19:39:03 -0700 Subject: [PATCH 4/5] Reflect comment on checks in promote_learner --- src/progress.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/progress.rs b/src/progress.rs index 04b059a16..bceea8171 100644 --- a/src/progress.rs +++ b/src/progress.rs @@ -151,14 +151,15 @@ impl ProgressSet { if self.voters.contains_key(&id) { Err(Error::Exists(id, "voters"))?; } - if !self.learners.contains_key(&id) { - Err(Error::NotExists(id, "learners"))? - } - if let Some(mut learner) = self.learners.remove(&id) { + // We don't want to remove it unless it's there. + if self.learners.contains_key(&id) { + let mut learner = self.learners.remove(&id).unwrap(); // We just checked! learner.is_learner = false; self.voters.insert(id, learner); + Ok(()) + } else { + Err(Error::NotExists(id, "learners")) } - Ok(()) } } From b0004c2fe29e2c4c8b6d79b6b679711c8bb5c410 Mon Sep 17 00:00:00 2001 From: Hoverbear Date: Tue, 14 Aug 2018 19:42:54 -0700 Subject: [PATCH 5/5] Satisfy clippy --- src/log_unstable.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/log_unstable.rs b/src/log_unstable.rs index 089f98b9c..a16d2c34c 100644 --- a/src/log_unstable.rs +++ b/src/log_unstable.rs @@ -217,8 +217,8 @@ mod test { for (entries, offset, snapshot, wok, windex) in tests { let u = Unstable { entries: entries.map_or(vec![], |entry| vec![entry]), - offset: offset, - snapshot: snapshot, + offset, + snapshot, ..Default::default() }; let index = u.maybe_first_index(); @@ -244,8 +244,8 @@ mod test { for (entries, offset, snapshot, wok, windex) in tests { let u = Unstable { entries: entries.map_or(vec![], |entry| vec![entry]), - offset: offset, - snapshot: snapshot, + offset, + snapshot, ..Default::default() }; let index = u.maybe_last_index(); @@ -305,8 +305,8 @@ mod test { for (entries, offset, snapshot, index, wok, wterm) in tests { let u = Unstable { entries: entries.map_or(vec![], |entry| vec![entry]), - offset: offset, - snapshot: snapshot, + offset, + snapshot, ..Default::default() }; let term = u.maybe_term(index); @@ -402,9 +402,9 @@ mod test { for (entries, offset, snapshot, index, term, woffset, wlen) in tests { let mut u = Unstable { - entries: entries, - offset: offset, - snapshot: snapshot, + entries, + offset, + snapshot, ..Default::default() }; u.stable_to(index, term); @@ -469,9 +469,9 @@ mod test { for (entries, offset, snapshot, to_append, woffset, wentries) in tests { let mut u = Unstable { - entries: entries, - offset: offset, - snapshot: snapshot, + entries, + offset, + snapshot, ..Default::default() }; u.truncate_and_append(&to_append);