fix: Change Redis.go to support explicitely connecting to a Redis Cluster with a single address for GCP compatibility #450
Conversation
…ster with a single address (GCP Model)
|
/gcbrun |
|
I still need to validate this works with Metastore - Redis Cluster in GCP to ensure it works well. |
internal/pkg/cache/redis/redis.go
Outdated
| @@ -29,6 +29,11 @@ type Redis struct { | |||
| c redis.UniversalClient | |||
| } | |||
|
|
|||
| const ( | |||
| redisClusterPrefix = "cluster/" | |||
There was a problem hiding this comment.
I do not like the implicit prefix hidden in the connection string.
I prefer if you provide two explicit New functions, one for creating a redis server and another for creating a redis cluster.
internal/pkg/cache/redis/redis.go
Outdated
| ConnMaxLifetime: cfg.MaxConnAge, | ||
| var c redis.UniversalClient | ||
|
|
||
| if isRedisClusterAddress(cfg.Address) { |
There was a problem hiding this comment.
Please use a different config variable to indicate to the service whether or not the redis server is in cluster mode or not.
Zurvarian
changed the title
internal/pkg/cache/redis/redis.go
Outdated
|
|
||
| if cfg.RedisMode == RedisModeCluster { | ||
| o := &redis.ClusterOptions{ | ||
| Addrs: parseRedisClusterMultiAddress(cfg.Address), |
There was a problem hiding this comment.
How is this working with the Redis Cluster discovery endpoint? Is the discovery endpoint a list of IPs, or is it always one IP?
There was a problem hiding this comment.
https://cloud.google.com/memorystore/docs/cluster/cluster-node-specification#discovery_endpoint
TL;DR: The discovery endpoint guarantees a consistent IP for the entire lifecycle of the Redis Cluster, independently of the nodes going up or down. Once the client connects to the endpoint, the client starts receiving packets with the master and replica nodes it must create a connection with.
…pected when configuring Redis
What type of PR is this?
/kind fix
What this PR does / Why we need it:
When testing Redis connection with GCP managed Redis Cluster we've seen that GCP expects clients to connect using a single discovery IP instead of the set of IPs that is expected by Go-Redis UniversalClient.
Which issue(s) this PR fixes:
Closes #449
Special notes for your reviewer: