Pool connections recycling #373
Conversation
|
Also you can see original SQLAlchemy code: https://github.com/zzzeek/sqlalchemy/blob/master/lib/sqlalchemy/pool.py#L623 |
Codecov Report
@@ Coverage Diff @@
## master #373 +/- ##
==========================================
+ Coverage 93.49% 93.53% +0.04%
==========================================
Files 23 23
Lines 3550 3572 +22
Branches 205 206 +1
==========================================
+ Hits 3319 3341 +22
Misses 186 186
Partials 45 45
Continue to review full report at Codecov.
|
aiopg/pool.py
Outdated
| @@ -187,6 +190,10 @@ def _fill_free_pool(self, override_min): | |||
| conn = self._free[-1] | |||
| if conn.closed: | |||
| self._free.pop() | |||
| elif self._recycle > -1 \ | |||
| and time.time() - conn.last_usage > self._recycle: | |||
There was a problem hiding this comment.
Should be self._loop.time(): asyncio uses time.monotonic() internally
|
@asvetlov I've made changes you requested |
|
Your PR is very similar to my own, https://github.com/aio-libs/aiopg/pull/366/files, except for where you're doing the time setting and resets. Yours has only 1 place where you set and reset the time, and truly if that's sufficient I'm perfectly happy with that. I'd only suggest you name the option |
|
@DanCardin I thought about |
|
right, but they're all in this PR, right? If you're only talking about renaming things in this PR, I'd be happy to do it. But I feel like maintaining parity between option(s) names definitely has value, since this is meant to look like sqlalchemy. |
…engine parameter See: aio-libs#373
|
Ok, @DanCardin - you've convinced me. |
|
@asvetlov We really need this! |
|
I will take a look also |
| @@ -17,20 +17,21 @@ | |||
|
|
|||
|
|
|||
| def create_pool(dsn=None, *, minsize=1, maxsize=10, | |||
| loop=None, timeout=TIMEOUT, | |||
| loop=None, timeout=TIMEOUT, pool_recycle=-1, | |||
There was a problem hiding this comment.
We need to add check for pool_recycle validation, like we do for minsize, also what do you think about making default value None or large number, say 3600 seconds?
There was a problem hiding this comment.
@jettify
When you have database on localhost - looks like you don't need connection recycling. I wasn't able to reproduce this issue on localhost. So, large values weren't needed by default.
What about -1 - I use this value only for compatibility with SQLAlchemy. Why did they use -1 - I don't know.
| @@ -187,6 +189,10 @@ def _fill_free_pool(self, override_min): | |||
| conn = self._free[-1] | |||
| if conn.closed: | |||
| self._free.pop() | |||
| elif self._recycle > -1 \ | |||
There was a problem hiding this comment.
I think this check a bit fragile, what will happened if someone pass pool_recycle=-0.5, I suggest change default and add parameter validation.
There was a problem hiding this comment.
I've used code logic from SQLAlchemy for compatibility - see: https://github.com/zzzeek/sqlalchemy/blob/master/lib/sqlalchemy/pool.py#L623
But I also think, that None by default and comparision self._recycle is not None and self._recycle > 0 will be better.
|
I like this PR, if my comment addressed I think we can merge. @asvetlov any objections? |
|
What do you think about this conversation? I can rewrite code to use |
|
thanks! |
|
New version available on pypi https://pypi.python.org/pypi/aiopg/0.13.1 |
|
@soar Thanks you for contribution! |
Related to issue: #372
Now you can do simply:
... and then all connections which were unused last
300seconds - will be automatically closed and re-established.