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

Use LIFO for returning/retrieving connections from the pool #2085

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

4ver
Copy link

@4ver 4ver commented Aug 16, 2018

The current way this works (cycling through all of the connections) means that you can't make use of mysql's wait_timeout setting for closing inactive connections. Once a connection is created it will be persisted forever unless you have very low levels of activity.

Was there a reason to cycle round-robin through the connections when this was initially implemented?

@4ver 4ver changed the title Use LIFO for returning/retrieving connections from pool Use LIFO for returning/retrieving connections from the pool Aug 16, 2018
Copy link
Member

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

Please add this as a configurable option. A pool representing a read slave through a tcp load balancer works better with rr for instance, so there are likely folks who are relying on the current behavior. Both behaviors are valid, so it makes to add a configuration to choose.

@4ver
Copy link
Author

4ver commented Aug 16, 2018

Will do 👍

@dougwilson
Copy link
Member

As for why it was initially implemented as rr it was before my time so I have no idea :)

@dougwilson
Copy link
Member

The implementation and test itself look good, though 👍

@4ver
Copy link
Author

4ver commented Aug 16, 2018

The test failures are a little funny, but it looks like I may have broken the PoolCluster RR stuff.

@4ver
Copy link
Author

4ver commented Aug 17, 2018

Must have just been a randomly failing test. Should be good to go now 🙆

@4ver
Copy link
Author

4ver commented Aug 17, 2018

#1749 can probably be closed off if this pr gets merged.

@4ver
Copy link
Author

4ver commented Aug 23, 2018

Gentle bump on this 😃. Had a chance to take a look over it @dougwilson ?

@dougwilson
Copy link
Member

Not since my initial look as I have been away. I will get back soon and take a look at the new changes

@4ver
Copy link
Author

4ver commented Aug 23, 2018

All good 👍 Thanks for the message.

@dougwilson dougwilson force-pushed the master branch 2 times, most recently from 946727b to 37fbbdd Compare November 18, 2018 02:26
@DiegoAlanis
Copy link

Did you get a chance to take a look over it? @dougwilson
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants