Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pool_recycle keyword argument to Pool. #366

Closed
wants to merge 1 commit into from
Closed

Add pool_recycle keyword argument to Pool. #366

wants to merge 1 commit into from

Conversation

DanCardin
Copy link

@DanCardin DanCardin commented Aug 7, 2017

Add a pool_recycle option similar to the optional available in sqlalchemy

aiopg/pool.py Outdated

# Close connections that have lived longer than the recycle limit.
if conn in self._free_timestamps:
now = datetime.datetime.now()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use loop.time() instead

aiopg/pool.py Outdated
self._conn_kwargs = kwargs
self._acquiring = 0
self._free = collections.deque(maxlen=maxsize or None)
self._free_timestamps = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest storing timestamp into connection object

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about that, but Connection doesn't know whether it's free or not at the moment, that I can tell. You'd be happy with an additional attribute which only gets updated through the pool?

I was going to add a free method to Connection to handle updating the time

@codecov
Copy link

codecov bot commented Aug 7, 2017

Codecov Report

Merging #366 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #366      +/-   ##
==========================================
+ Coverage   93.49%   93.54%   +0.05%     
==========================================
  Files          23       23              
  Lines        3550     3578      +28     
  Branches      205      208       +3     
==========================================
+ Hits         3319     3347      +28     
  Misses        186      186              
  Partials       45       45
Impacted Files Coverage Δ
aiopg/pool.py 98.94% <100%> (+0.04%) ⬆️
tests/test_pool.py 99.75% <100%> (ø) ⬆️
aiopg/sa/connection.py 91.52% <0%> (+0.17%) ⬆️

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 7df6708...d843742. Read the comment docs.

@DanCardin
Copy link
Author

Just to note, I've made changes according to the comments/requested changes, though I'm not sure if this is 100% what you meant slash if there are other places or concerns that I've missed which could get the connection out of sync with the pool

@DanCardin
Copy link
Author

closing because #373 seems to do the same thing better!

@DanCardin DanCardin closed this Sep 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants