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 remove to connection pool Lease #4681

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kdubb
Copy link

@kdubb kdubb commented Apr 24, 2023

Implements #4680

@vietj
Copy link
Member

vietj commented Apr 24, 2023

it seems that this operation can break pool invariants, e.g if the connection has several leases, when another lease calls recycle() then no operation should be done since the slot might hold a different connection, e.g it could call slot.usage-- and we assume this is possible because it has increased the slot usage before with the same connection

@vietj
Copy link
Member

vietj commented Apr 24, 2023

as I understand you would like to mark a connection as stale so it should never be borrowed again from the pool and eventually becomes removed from the pool to let another connection to be created ?

@kdubb
Copy link
Author

kdubb commented Apr 24, 2023

This probably should have been a draft because I don't quite understand all the inner workings of the pool slots.

as I understand you would like to mark a connection as stale so it should never be borrowed again from the pool and eventually becomes removed from the pool to let another connection to be created ?

Essentially yes, we need a way to communicate to the pool that the connection should be removed.

@vietj
Copy link
Member

vietj commented Apr 24, 2023

I think then we need a new shutdown operation on ConnectionPool that performs this that will set a shutdown flag on the slot and have the pool not check to lease this again when the flag is true, pretty much like:

void shutdown(Predicate<C> pred, Supplier<C> closer)

It would be called like pool.shutdown(conn -> conn.isValid(), conn -> conn.close())

when a connection is marked as shutdown, the slot.usage can only decrease and when it reaches 0 then it will be simply removed from the pool an then closed, another connection can be created to satisfy the waiters then.

@chandramouli-r
Copy link

chandramouli-r commented Jun 12, 2023

@vietj @tsegismont I tried to implement this idea as a pre-cursor to maintaining minIdle connections in the pool.

Basically,

  1. Add a maxTttlMs option to the pool
  2. Add a inactive flag to the slot (boolean), and createTimeMs field to the slot.
  3. createTimeMs is initialized when a connection is created in a slot
  4. In recycle, check if the maxTtlMs has expired for the slot. If yes, and if the slot usage is 0, remove the connection and set inactive = true. If not, only set inactive = true.
  5. In acquire, add a check to the Selector to not select any slot that has inactive = true
  6. When a new connection is added to the slot, change inactive back to false, reset createTimeMs

Then, during pool initialization and during remove, we need to call a method that refreshes the pool with minIdle connections (not part of the gist).

If this looks good to you, I can create a PR. Please let me know.

https://gist.github.com/chandramouli-r/f8adf3b765919ceb54dea9126a2d97f2

@chandramouli-r
Copy link

@vietj @tsegismont create a draft PR for this: #4736 (the added API can be used in SqlConnectionPool for checking maxLifetime when a connection is returned back to the pool).

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.

3 participants