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

allow more redis connection options #419

Merged
merged 1 commit into from
May 23, 2023
Merged

allow more redis connection options #419

merged 1 commit into from
May 23, 2023

Conversation

metachris
Copy link
Collaborator

πŸ“ Summary

Inspired by https://github.com/bloXroute-Labs/mev-relay/blob/develop/datastore/redis.go


βœ… I have run these commands

  • make lint
  • make test-race
  • go mod tidy
  • I have seen and agree to CONTRIBUTING.md

@codecov-commenter
Copy link

Codecov Report

Merging #419 (7aae7f4) into main (eec12f7) will decrease coverage by 0.14%.
The diff coverage is 18.75%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #419      +/-   ##
==========================================
- Coverage   30.77%   30.63%   -0.14%     
==========================================
  Files          24       24              
  Lines        4569     4579      +10     
==========================================
- Hits         1406     1403       -3     
- Misses       2987     2996       +9     
- Partials      176      180       +4     
Flag Coverage Ξ”
unittests 30.63% <18.75%> (-0.14%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Ξ”
datastore/redis.go 61.90% <18.75%> (-2.37%) ⬇️


if redisConnectionPoolSize > 0 {
redisOpts.PoolSize = redisConnectionPoolSize
redisOpts.MinIdleConns = redisConnectionPoolSize / 2

Choose a reason for hiding this comment

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

You might want to add some more information about this value. If the PoolSize value is high and the redis service is shared across multiple instances you could end up maxing out the available connections on redis and see random connection errors.

Results from a quick google, but probably better resources out there to explain this:

Redis nodes can have up to either 10,000 simultaneous connections or 4 simultaneous connections per megabyte of memory, whichever is larger.

Copy link
Collaborator Author

@metachris metachris May 20, 2023

Choose a reason for hiding this comment

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

Good idea. What I could find was

In Redis 2.6 and newer, this limit is dynamic: by default it is set to 10000 clients, unless otherwise stated by the maxclients directive in redis.conf.

However, Redis checks with the kernel what the maximum number of file descriptors that we are able to open is (the soft limit is checked). If the limit is less than the maximum number of clients we want to handle, plus 32 (that is the number of file descriptors Redis reserves for internal uses), then the maximum number of clients is updated to match the number of clients it is really able to handle under the current operating system limit.

https://redis.io/docs/reference/clients/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe it would be better to configure poolSize and minIdleConnections separately πŸ€”

@metachris metachris merged commit e39cd38 into main May 23, 2023
4 checks passed
@metachris metachris deleted the redis-updates branch May 23, 2023 11:07
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.

None yet

3 participants