-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Support Redis Cluster Configuration Endpoint #4947
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @siavashs for the PR 👍 . LGTM
Left some minor suggestions.
@@ -51,9 +52,27 @@ type RedisClient struct { | |||
} | |||
|
|||
// NewRedisClient creates Redis client | |||
func NewRedisClient(cfg *RedisConfig) *RedisClient { | |||
func NewRedisClient(cfg *RedisConfig) (*RedisClient, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think would be nice to add few simples tests to lock this behaviour in NewRedisClient
with different values for cfg.Endpoint
(single vs multiple comma separated strings?)
We may not need to patch Go resolver during runtime (like you mentioned on the PR description). How about just passing list comma separated IPs and not hostnames? Will that help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have 2 tests here which pass a single endpoint and then 2 endpoints as a cluster using mock redis instances: https://github.com/grafana/loki/blob/main/pkg/storage/chunk/cache/redis_client_test.go#L67-L100
I assume these are passing IPs to the client config, so they should already cover what you mentioned.
The go-redis options requires multiple redis addresses for a cluster. In some scenarios like AWS ElastiCache, a single configuration endpoint is used to resolve all cluster nodes. This change adds an extra step to resolve all nodes and pass them to go-redis. It remove the need to specify multiple cluster nodes in the configuration. No tests were added for this scenario since it requires patching the Go resolver during runtime. Signed-off-by: Siavash Safi <siavash.safi@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from my side!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs change LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What this PR does / why we need it:
The go-redis options requires multiple redis addresses for a cluster.
In some scenarios like AWS ElastiCache, a single configuration endpoint is used to resolve all cluster nodes.
This change adds an extra step to resolve all nodes and pass them to go-redis.
It removes the need to specify multiple cluster nodes in the configuration.
Signed-off-by: Siavash Safi siavash.safi@gmail.com
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
No tests were added for this scenario since it requires patching the Go resolver during runtime.
But let me know if you have a suggestion.
Same change for Cortex cortexproject/cortex#4590
Checklist
CHANGELOG.md
about the changes.