A way to understand whether Client is a clusterClient

[FZambia]

Hello!

In Centrifuge/Centrifugo I used to require explicit Redis Cluster configuration (i.e. telling in the configuration that the Redis setup is cluster), because in Cluster mode Centrifuge library starts generating keys a bit differently (using hash tags).

But it seems it's not very convenient because Cloud providers provide Redis URL like redis://... and under the hood can be standalone Redis or Redis Cluster. So users can't just re-use endpoint for starting Centrifugo.

I now decided that it's time to detect whether it's cluster or not automatically, but the only way I found to understand whether the underlying rueidis Client is a clusterClient is sth like this:

client, _ = rueidis.NewClient(...)
if strings.Contains(strings.ToLower(fmt.Sprintf("%T", client)), "cluster") {
  // Redis is a Redis Cluster.
}

I.e. looking at underlying interface type. Which is of course hacky.

I could issue extra command myself to Redis, but I think it's not a good idea given how many Redis implementations we have these days – so would prefer relying on rueidis logic. Also, I can not always generate keys in Redis Cluster format for keeping backwards compatibility and having a bit more clean keys in non-cluster scenario.

Maybe I missed some way? If not - can we add sth native for doing such a check? Given rueidis.Client is an interface, what would be a preferred way to support the check of underlying client natively in rueidis?

[rueian]

Hi @FZambia,

How about adding a new Mode() function to the interface like this:

type Mode string
// https://github.com/valkey-io/valkey/blob/1a34a4ff7f101bb6b17a0b5e9aa3bf7d6bd29f68/src/networking.c#L4118-L4124
const (
	ModeCluster    Mode = "cluster"
	ModeSentinel   Mode = "sentinel"
	ModeStandalone Mode = "standalone"
)

type Client interface {
	...
	Mode() Mode
}

[rueian]

Each client implementation will return the corresponding constant. No additional cost.

[FZambia]

Works for me definitely, the reason why I asked regarding interface is that changing Client interface is potentially a backwards incompatible change.

I personally was thinking about sth like this:

// ClientMode represents the mode of the client. See ClientModeCluster, ClientModeSentinel, ClientModeStandalone.
type ClientMode string

// Known client modes.
// See https://github.com/valkey-io/valkey/blob/1a34a4ff7f101bb6b17a0b5e9aa3bf7d6bd29f68/src/networking.c#L4118-L4124.
const (
	ClientModeCluster    ClientMode = "cluster"
	ClientModeSentinel   ClientMode = "sentinel"
	ClientModeStandalone ClientMode = "standalone"
)

// ClientModeGetter is the interface to get the client mode from Client.
// The reason why a separate interface is used is to keep Client interface backwards compatible.
// All clients – singleClient, clusterClient and sentinelClient implement it.
type ClientModeGetter interface {
	GetMode() ClientMode
}

// Then in clusterClient.
var _ ClientModeGetter = (*clusterClient)(nil)

func (c *clusterClient) GetMode() ClientMode {
	return ClientModeCluster
}

// Same for single, sentinel

Usage:

client, _ := rueidis.NewClient(options)
if modeGetter, ok := client.(rueidis.ClientModeGetter); ok {
	mode := modeGetter.GetMode()
	fmt.Println("Redis mode:", mode)
}

But maybe you think extending an interface is better and it's safe for users. Again - for me both options work. Just worrying to not introduce breaking changes

[rueian]

Works for me definitely, the reason why I asked regarding interface is that changing Client interface is potentially a backwards incompatible change.

Yes, it is a backwards incompatible change. However, requiring users to cast the Client to another interface is too inconvenient. I believe adding a method usually won't cause trouble to users unless they implement the interface by themselves.

I think we can finalize the interface to be

type ClientMode string

const (
	ClientModeCluster    ClientMode = "cluster"
	ClientModeSentinel   ClientMode = "sentinel"
	ClientModeStandalone ClientMode = "standalone"
)

type Client interface {
	...
	Mode() ClientMode
}

I just feel GetMode() is a bit verbose, but type ClientMode string is more clear!

[CheyuWu]

Hi @rueian May I help with this?