UniversalClient problems with defining Cluster/Simple types

[swdee]

We want our application code base to support connection to both Simple (single standalone) and Cluster Redis servers. Currently we have implemented this with our own wrapper for both NewClusterClient() and NewClient(), however I see you have a UniversalClient implementation that achieves the same thing as our wrapper.

In the documentation for NewUniversalClient() it states how the Client Kind is selected.

1. if a MasterName is passed a sentinel-backed FailoverClient will be returned
2. if the number of Addrs is two or more, a ClusterClient will be returned
3. otherwise, a single-node redis Client will be returned.

This logic can be seen at the code https://github.com/go-redis/redis/blob/30e120f33e35ce9a1043d405cdb70305cb6734bf/universal.go#L198-L205

It would be cleaner for us to stop using our wrapper and use the UniversalClient however the above automatic logic used to define which type of client is returned is a problem for the following reason. Our redis cluster is deployed within Kubernetes and only has a single load balanced address to connect to for the initial cluster topology to be established. It would be better if the automatic logic above was removed and for the Client kind to be specified as a required parameter in the function.

We propose the following;

type ClientKind int

const (
	Cluster ClientKind = iota
	Failover
	Simple
)

func NewUniversalClient(kind ClientKind, opts *UniversalOptions) UniversalClient {
	switch kind {
	case Cluster:
		return NewClusterClient(opts.Cluster())

	case Failover:
		return NewFailoverClient(opts.Failover())

	case Simple:
		fallthrough
	default:
		return NewClient(opts.Simple())
	}
}

What are your thoughts on this proposal? It would have to be for a v9 release as this would be a breaking change to the existing API, unless the above was introduced under a different function name like NewUniversalClientKind()?

[vmihailenco]

@swdee what about a new option ClientKind in UniversalOptions? Then NewUniversalClient could use it when available.

[swdee]

@vmihailenco That would work also.

[ppai-plivo]

AWS ElastiCache's clustered redis exposes a configuration endpoint which is a DNS A record that has IPs of all nodes. Since this endpoint is a single address, NewUniversalClient would return a non-clustered client instead of the desired clustered one. The proposal to add a new field to UniversalOptions also helps this usecase 👍

However, to retain the current behaviour in v8, the default (0 value) of this field needs to be a no-op i,e fallback to current code.

type ClientKind int

const (
	Cluster ClientKind = iota + 1
	Failover
	Simple
)

func NewUniversalClient(opts *UniversalOptions) UniversalClient {
	switch opts.ClientKind {
	case Cluster:
		return NewClusterClient(opts.Cluster())
	case Failover:
		return NewFailoverClient(opts.Failover())
	case Simple:
		return NewClient(opts.Simple())
	default:
	}

	if opts.MasterName != "" {
		return NewFailoverClient(opts.Failover())
	} else if len(opts.Addrs) > 1 {
		return NewClusterClient(opts.Cluster())
	}

	return NewClient(opts.Simple())
}

[AnoopPutta]

@ppai-plivo hi, can i use the clustered client to connect to AWS ElastiCache's clustered redis which exposes a configuration endpoint which is a DNS A record that has IPs of all nodes? Please advice.

[github-actions[bot]]

This issue is marked stale. It will be closed in 30 days if it is not updated.

[kameshraoyeduvakula]

@ppai-plivo hi, can i use the clustered client to connect to AWS ElastiCache's clustered redis which exposes a configuration endpoint which is a DNS A record that has IPs of all nodes? Please advice.

whats the answer to this?