Pool connections recycling#373

soar · 2017-08-30T10:12:08Z

Related to issue: #372

Now you can do simply:

db_engine = await aiopg.sa.create_engine(..., recycle=300)

... and then all connections which were unused last 300 seconds - will be automatically closed and re-established.

soar · 2017-08-30T10:14:19Z

Also you can see original SQLAlchemy code: https://github.com/zzzeek/sqlalchemy/blob/master/lib/sqlalchemy/pool.py#L623

codecov · 2017-08-30T10:20:43Z

Codecov Report

Merging #373 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            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

Impacted Files	Coverage Δ
aiopg/sa/engine.py	`100% <ø> (ø)`	⬆️
tests/test_pool.py	`99.75% <100%> (ø)`	⬆️
aiopg/pool.py	`98.91% <100%> (+0.02%)`	⬆️
aiopg/sa/connection.py	`91.46% <0%> (+0.11%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a23294c...8f5deab. Read the comment docs.

asvetlov · 2017-08-30T16:34:30Z

            if conn.closed:
                self._free.pop()
+            elif self._recycle > -1 \
+                    and time.time() - conn.last_usage > self._recycle:


Should be self._loop.time(): asyncio uses time.monotonic() internally

@asvetlov Done

soar · 2017-08-30T18:38:20Z

@asvetlov I've made changes you requested

DanCardin · 2017-09-01T11:52:03Z

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 pool_recycle to match the option in sqlalchemy's create_engine call.

soar · 2017-09-01T12:12:33Z

@DanCardin I thought about pool_recycle, but there are too many places to rename this parameter.

DanCardin · 2017-09-01T12:53:35Z

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

soar · 2017-09-01T17:43:42Z

Ok, @DanCardin - you've convinced me.

soar · 2017-09-06T08:33:26Z

@asvetlov We really need this!

jettify · 2017-09-06T10:34:46Z

I will take a look also

jettify · 2017-09-06T20:41:31Z


 def create_pool(dsn=None, *, minsize=1, maxsize=10,
-                loop=None, timeout=TIMEOUT,
+                loop=None, timeout=TIMEOUT, pool_recycle=-1,


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?

@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.

jettify · 2017-09-06T20:43:02Z

            conn = self._free[-1]
            if conn.closed:
                self._free.pop()
+            elif self._recycle > -1 \


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.

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.

make sense, lets leave -1

jettify · 2017-09-06T20:44:46Z

I like this PR, if my comment addressed I think we can merge. @asvetlov any objections?

soar · 2017-09-07T08:29:58Z

@jettify @DanCardin

What do you think about this conversation? I can rewrite code to use None by default, but this won't be compatible with SQLAlchemy parameters.

jettify · 2017-09-07T11:32:54Z

@soar lets give @asvetlov few days to look, otherwise I can merge this weekend.

jettify · 2017-09-10T19:22:29Z

thanks!

jettify · 2017-09-10T20:27:16Z

New version available on pypi https://pypi.python.org/pypi/aiopg/0.13.1

soar · 2017-09-11T07:14:58Z

@jettify @asvetlov Thank you!

jettify · 2017-09-11T07:32:16Z

@soar Thanks you for contribution!

soar added 2 commits August 30, 2017 12:38

Added optional parameter recycle for create_engine in aiopg.sa

7aae914

Fix code style

c622443

soar added 2 commits August 30, 2017 15:57

Added test for connection recycling

54eea1f

Use loop.time() instead of time.time()

0d006db

asvetlov requested changes Aug 30, 2017

View reviewed changes

soar added 2 commits August 30, 2017 21:18

One more time.time() to loop.time() change

cfc3b7e

Remove unused import

ca36327

Rename parameter recycle to pool_recycle according to SQLAlchemy …

8f5deab

…engine parameter See: aio-libs#373

soar mentioned this pull request Sep 1, 2017

connection recycling soar/aiopg#1

Merged

DanCardin mentioned this pull request Sep 1, 2017

Add pool_recycle keyword argument to Pool. #366

Closed

jettify reviewed Sep 6, 2017

View reviewed changes

asvetlov approved these changes Sep 10, 2017

View reviewed changes

jettify merged commit 5699f8d into aio-libs:master Sep 10, 2017

This was referenced Oct 15, 2017

Pool recylce aio-libs/aiomysql#216

Merged

psycopg2.OperationalError: server closed the connection unexpectedly #372

Closed

drpoggi mentioned this pull request Feb 22, 2018

Add optional pool connections recycling aio-libs/aioodbc#167

Merged

Uh oh!

Conversation

soar commented Aug 30, 2017

Uh oh!

soar commented Aug 30, 2017

Uh oh!

codecov bot commented Aug 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soar commented Aug 30, 2017

Uh oh!

DanCardin commented Sep 1, 2017

Uh oh!

soar commented Sep 1, 2017

Uh oh!

DanCardin commented Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soar commented Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soar commented Sep 6, 2017

Uh oh!

jettify commented Sep 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jettify commented Sep 6, 2017

Uh oh!

soar commented Sep 7, 2017

Uh oh!

jettify commented Sep 7, 2017

Uh oh!

jettify commented Sep 10, 2017

Uh oh!

jettify commented Sep 10, 2017

Uh oh!

soar commented Sep 11, 2017

Uh oh!

jettify commented Sep 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Aug 30, 2017 •

edited

Loading

DanCardin commented Sep 1, 2017 •

edited

Loading

soar commented Sep 1, 2017 •

edited

Loading