From 2b7977a47bd4f7e6bdfd7f5cf55a9ff06a1ceb78 Mon Sep 17 00:00:00 2001 From: Hoverbear Date: Wed, 8 Aug 2018 11:36:09 -0700 Subject: [PATCH] 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 8b86d9cab..87af775a7 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; } @@ -1854,7 +1858,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; } @@ -1905,9 +1911,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 5b385c548..7c5813a87 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;