feat: add max_lifetime_jitter
#3292
Conversation
| @@ -24,6 +24,7 @@ pub struct PoolConnection<DB: Database> { | |||
| pub(super) struct Live<DB: Database> { | |||
| pub(super) raw: DB::Connection, | |||
| pub(super) created_at: Instant, | |||
| pub(super) max_lifetime: Option<Duration>, | |||
There was a problem hiding this comment.
My biggest concern is this field increasing the size of PoolConnection. The size of connection structs is already a concern (#3265) and this is going to add 16 whole bytes.
Instead of storing it with a connection, you could just calculate it every time you check a connection's age against max_lifetime. My gut tells me it won't affect performance that much.
Rolling the dice every time does probably change the distribution, but I think it would bias towards closing connections that have been open past max_lifetime the longest, which seems reasonable. I think it'll still be random enough to mitigate the thundering herd issue.
| /// | ||
| /// It's recommended to set this to 10% of the `max_lifetime` time for production | ||
| /// workloads. | ||
| pub fn max_lifetime_jitter(mut self, lifetime: impl Into<Option<Duration>>) -> Self { |
There was a problem hiding this comment.
Instead of taking a duration and advising the user roughly what value to set, I would just take a float and default it to 0.1. It doesn't need to take Option, just don't apply jitter if it's set to 0.
That way they also don't have to update it if they change max_lifetime, and this can even benefit users who don't think to take advantage of it.
I don't think anyone is going to care about being able to set any exact value other than 0 anyway.
f32 is fine, it probably won't even increase the size of the struct since it can likely just replace some padding.
I would also have it only increase max_lifetime so you don't have to worry about clamping the result, and it simplifies the math. There's not much benefit to making it shorter anyway.
| let jitter = jitter.min(max_lifetime); | ||
|
|
||
| let mut rng = rand::thread_rng(); | ||
| let jitter_value = rng.gen_range((-jitter.as_secs_f64())..=jitter.as_secs_f64()); | ||
|
|
||
| if jitter_value >= 0. { | ||
| Some(max_lifetime + Duration::from_secs_f64(jitter_value)) | ||
| } else { | ||
| Some(max_lifetime.saturating_sub(Duration::from_secs_f64(jitter_value.abs()))) | ||
| } |
There was a problem hiding this comment.
If you apply the above suggestions, this could be as simple as:
let jitter = (self.max_lifetime_jitter * rand::random::<f32>()) + 1;
max_lifetime.mul_f32(jitter)
|
@NathanFlurry have you seen my comments? |
|
Closing due to inactivity. Feel free to open a new PR if you get time to address my comments. |
Using SQLx under load causes a stampeding herd of SQL connections every
max_lifetime. This helps resolve that problem by optionally adding a jitter to the max lifetime.Fixes #2854
Related #3057