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 support for MinConnections to thriftbp.ClientPool #634

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

Conversation

m3lawren
Copy link

@m3lawren m3lawren commented Nov 9, 2023

💸 TL;DR

This adds the ability to specify a minimum number of connections to thriftbp.ClientPool and clientpool.ChannelPool. Having a maintained minimum pool size provides benefits for load balancing which can help even out CPU/RAM utilization on the server side.

📜 Details

We can't rely on requiredInitialClients (since it can fail startup) and if bestEffortInitialClients fails we end up with a partly preallocated pool, this provides a mechanism to maintain the pool size on an ongoing basis.

This is accomplished by spinning up an ensureMinClients goroutine per pool which will check the total number of connections (active+idle) and open more as needed to keep that amount >= the minimum.

It should be non-breaking as the default behaviour is to not have a minimum set and not spin up the ensureMinClients goroutine.

🧪 Testing Steps / Validation

✅ Checks

  • CI tests (if present) are passing
  • Adheres to code style for repo
  • Contributor License Agreement (CLA) completed if not a Reddit employee


func (cp *channelPool) ensureMinClients() {
for !cp.isClosed.Load() {
time.Sleep(cp.backgroundTaskInterval)
Copy link
Member

Choose a reason for hiding this comment

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

create a time.Ticker to loop through instead of using sleep, you can use closed signal as the condition of termination of the loop.

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

Successfully merging this pull request may close these issues.

2 participants