FSX / momoko

Wraps (asynchronous) Psycopg2 for Tornado.
http://momoko.61924.nl/
Other
363 stars 73 forks source link

Ability to add auto shrinking of connection pool size #108

Closed jchumnanvech closed 9 years ago

haizaar commented 9 years ago

Thanks for the code. Couple of things are missing though:

Please always write unittests for any features/bugs you submit and update the docs. The code also should pass PEP8 compliance with pep8 --max-line-length=120.

jchumnanvech commented 9 years ago

After every run of auto shrink it re-adds itself back to the ioloop.

I hesitate about waiting until x minutes after the last query because lets say we put pool size 5 and max to 20. We can anticipate constant connections around 3-4 but every now and then we get high traffic and we need to service 20 concurrent connections. The high traffic period ends and the constant traffic of 3-4 continues for the life time of the app. The connection pool will never shrink back down.

haizaar commented 9 years ago

My bad, you are right about the timeout.

I see what you are saying. I'm concerned about that fact that you want to shrink free connections without looking at the pool load. Even during high traffic faze, a connection can become free every now and then and there is a chance that it will be shrunk.

How about:

What do you think?

jchumnanvech commented 9 years ago

All your comments sound good. I went ahead and implemented them all.

I added in last_used_time and use that in the calculation of shrink_delay before removing it from the pool and closing the connection. I changed ConnectionPool.free to a deque as well.

I added in a unit test to make sure the pool shrank and made the code pass pep8 inspection. It still complains a little with pep8 about imports not being at the top but it does that on master as well. So no new inspection errors were created.

haizaar commented 9 years ago

Looks good to me modulo comments added. Please also update Pool docstring to include the new parameters.

Also I think it worth adding another unittest - stretch pool to 5, then keep running 3 queries continuously in parallel and verify that other 2 connections got shrunk after shrink_delay. This will verify that the idea with connection stickiness is actually working.

Thanks!

jchumnanvech commented 9 years ago

Did all the requests. Added the unit test.

haizaar commented 9 years ago

Thanks!