-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add jitter to constant backoff #47
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,13 +35,15 @@ use crate::backoff::BackoffBuilder; | |
pub struct ConstantBuilder { | ||
delay: Duration, | ||
max_times: Option<usize>, | ||
jitter: bool, | ||
} | ||
|
||
impl Default for ConstantBuilder { | ||
fn default() -> Self { | ||
Self { | ||
delay: Duration::from_secs(1), | ||
max_times: Some(3), | ||
jitter: false, | ||
} | ||
} | ||
} | ||
|
@@ -58,6 +60,12 @@ impl ConstantBuilder { | |
self.max_times = Some(max_times); | ||
self | ||
} | ||
|
||
/// Set jitter on | ||
pub fn with_jitter(mut self) -> Self { | ||
self.jitter = true; | ||
self | ||
} | ||
} | ||
|
||
impl BackoffBuilder for ConstantBuilder { | ||
|
@@ -69,6 +77,7 @@ impl BackoffBuilder for ConstantBuilder { | |
max_times: self.max_times, | ||
|
||
attempts: 0, | ||
jitter: self.jitter, | ||
} | ||
} | ||
} | ||
|
@@ -80,6 +89,7 @@ pub struct ConstantBackoff { | |
max_times: Option<usize>, | ||
|
||
attempts: usize, | ||
jitter: bool, | ||
} | ||
|
||
impl Default for ConstantBackoff { | ||
|
@@ -88,6 +98,7 @@ impl Default for ConstantBackoff { | |
delay: Duration::from_secs(1), | ||
max_times: Some(3), | ||
attempts: 0, | ||
jitter: false, | ||
} | ||
} | ||
} | ||
|
@@ -96,14 +107,18 @@ impl Iterator for ConstantBackoff { | |
type Item = Duration; | ||
|
||
fn next(&mut self) -> Option<Self::Item> { | ||
let delay = || match self.jitter { | ||
true => self.delay + self.delay.mul_f32(fastrand::f32()), | ||
false => self.delay, | ||
}; | ||
match self.max_times { | ||
None => Some(self.delay), | ||
None => Some(delay()), | ||
Some(max_times) => { | ||
if self.attempts >= max_times { | ||
None | ||
} else { | ||
self.attempts += 1; | ||
Some(self.delay) | ||
Some(delay()) | ||
} | ||
} | ||
} | ||
|
@@ -146,4 +161,13 @@ mod tests { | |
assert_eq!(Some(Duration::from_secs(1)), exp.next()); | ||
assert_eq!(None, exp.next()); | ||
} | ||
|
||
#[test] | ||
fn test_constant_with_jitter() { | ||
let mut it = ConstantBuilder::default().with_jitter().build(); | ||
|
||
let dur = it.next().unwrap(); | ||
fastrand::seed(7); | ||
assert!(dur > Duration::from_secs(1)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's hard to test random things, this should be something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you willing to start a new PR for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe for unit test is enough. I will try to find a better solution, but I'm not sure. |
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In default case it's a little strange to set delay(1s) + jitter(1s in some cases), so double the main delay. But maybe it's ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add jitter range for user in the future, let's merge first.