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

Redis broker pool size #258

Closed
anhnguyensgu opened this issue Oct 6, 2022 · 4 comments
Closed

Redis broker pool size #258

anhnguyensgu opened this issue Oct 6, 2022 · 4 comments

Comments

@anhnguyensgu
Copy link

I have set up the package to handle the real-time message and it works perfectly. However, I am so curious about the pool size configuration for Redis broker. After taking a look at the code Redis_shard.go, I found that Redis broker takes the default pool size 128.

Do we have a way to configure the pool size?

@FZambia
Copy link
Member

FZambia commented Oct 6, 2022

Hello, @anhnguyensgu!

There are 3 modes in current Redis broker implementation:

  1. Standalone Redis
  2. Redis with Sentinel
  3. Redis Cluster

For cases 1 and 2 pool size does not really matter - we utilize Redis pipelining and only several active connections from Centrifuge to Redis are active at any point of time.

I think pool size only matters in Redis Cluster case - where we are using pool to issue most of commands except publish without history (which is pipelined).

I think in the near future Centrifuge Redis implementation may migrate from redigo to go-redis or possibly to rueidis library - and those allow using pipelining for communication with Redis Cluster too. Pool size should not be too important in that case I believe.

So I think we should not expose pool size at this point. Do you have issues with current default pool size (which I think may happen only in Redis Cluster scenario)? If yes - we can address it more precisely, possibly looking how go-redis from the mentioned pull request behaves - I think it should provide better overall throughput.

@anhnguyensgu
Copy link
Author

Thanks for your support. At this time, I have no issues with the pool, I will let you know if having any.

@FZambia
Copy link
Member

FZambia commented Nov 28, 2022

We migrated to rueidis library in #262 – where connection pool does not have much sense anymore, Centrifuge will use pipelining over several connections for all operations. So I suppose this issue can be closed.

@FZambia FZambia closed this as completed Nov 28, 2022
@FZambia
Copy link
Member

FZambia commented Nov 28, 2022

The draft of blog post about rueidis migration: centrifugal/centrifugal.dev#18 - @anhnguyensgu probably you will be interested to check it

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

No branches or pull requests

2 participants