Set reaper rate to minimum value #158
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #158 +/- ##
==========================================
+ Coverage 74.28% 75.08% +0.80%
==========================================
Files 6 6
Lines 556 562 +6
==========================================
+ Hits 413 422 +9
+ Misses 143 140 -3
☔ View full report in Codecov by Sentry. |
| @@ -183,6 +186,11 @@ impl<M: ManageConnection> Builder<M> { | |||
| Some(Duration::from_secs(0)), | |||
| "max_lifetime must be greater than zero!" | |||
| ); | |||
|
|
|||
| if let Some(max_lifetime) = max_lifetime { | |||
There was a problem hiding this comment.
This isn't very precise... if max_lifetime is 30s and the reaper rate is set to 30s as well, if the reaper comes along after 15s it won't reap and then it will be 45s before the max_lifetime is enforced. I'm inclined to think this is more like a documentation problem, and/or maybe we could emit some warnings if there is a large difference between these values?
There was a problem hiding this comment.
True. Alternatively, we can just run the reaper more frequently, e.g. PgBouncer runs its "janitor" 3 times per second.
There was a problem hiding this comment.
That's what the configuration is for. I'm comfortable with the current default configuration as a default, so the goal here is to try and make sure people don't get surprised by the interactions.
There was a problem hiding this comment.
Well... if we look at how the interface is, right now someone can set max_lifetime to 1s and nothing will happen for 30 seconds. So ideally, someone sets that setting to any value they want and the pool just works. reaper_rate is a leaked abstraction imo, it requires the user to understand how the pool works internally.
There was a problem hiding this comment.
I don't disagree that it's a leaky abstraction, my point is more that I don't think it's feasible to avoid the leakage in a fairly general-purpose abstraction like this. So instead of trying do so in a limited way which could be surprising in different ways, I'd prefer clearly documenting how these values work together.
Fix #157