Skip to content

feat: Add Mode() Method to rueidis.Client for Detecting Client Mode#794

Merged
rueian merged 8 commits intoredis:mainfrom
CheyuWu:feat/client/mode
Mar 7, 2025
Merged

feat: Add Mode() Method to rueidis.Client for Detecting Client Mode#794
rueian merged 8 commits intoredis:mainfrom
CheyuWu:feat/client/mode

Conversation

@CheyuWu
Copy link
Copy Markdown
Contributor

@CheyuWu CheyuWu commented Mar 6, 2025

Why do we need this?

Previously, Centrifugo required explicit Redis Cluster configuration because the Centrifuge library generates keys differently in cluster mode (using hash tags). However, this approach is inconvenient because cloud providers typically provide a Redis URL (e.g., redis://...) without explicitly indicating whether it is a standalone Redis or a Redis Cluster. This prevents users from simply reusing the endpoint when starting Centrifugo.

New Interface Definition

type ClientMode string

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

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

Closes #790

CheyuWu added 5 commits March 6, 2025 20:05
Signed-off-by: Cheyu Wu <cheyu1220@gmail.com>
Signed-off-by: Cheyu Wu <cheyu1220@gmail.com>
Comment thread rueidis.go Outdated
// https://github.com/valkey-io/valkey/blob/1a34a4ff7f101bb6b17a0b5e9aa3bf7d6bd29f68/src/networking.c#L4118-L4124
ModeCluster ClientMode = "cluster"
ModeSentinel ClientMode = "sentinel"
ModeStandalone ClientMode = "standalone"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should be with Client prefix:

ClientModeCluster ClientMode = "cluster"
ClientModeSentinel ClientMode = "sentinel"
ClientModeStandalone ClientMode = "standalone"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oops, I will fix this

@CheyuWu
Copy link
Copy Markdown
Contributor Author

CheyuWu commented Mar 6, 2025

also cc @rueian

Comment thread client.go Outdated
}

func (c *singleClient) Mode() ClientMode {
return c.mode
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return c.mode
return ClientModeStandalone

Comment thread client.go Outdated
cmd Builder
retry bool
DisableCache bool
mode ClientMode
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
mode ClientMode

Comment thread cluster.go Outdated
retry: !opt.DisableRetry,
retryHandler: retryer,
stopCh: make(chan struct{}),
mode: ClientModeCluster,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
mode: ClientModeCluster,

Comment thread cluster.go Outdated
}

func (c *clusterClient) Mode() ClientMode {
return c.mode
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return c.mode
return ClientModeCluster

Comment thread sentinel.go Outdated
retry: !opt.DisableRetry,
retryHandler: retryer,
replica: opt.ReplicaOnly,
mode: ClientModeSentinel,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
mode: ClientModeSentinel,

Comment thread sentinel.go Outdated
cmd Builder
retry bool
replica bool
mode ClientMode
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
mode ClientMode

Comment thread sentinel.go Outdated
}

func (c *sentinelClient) Mode() ClientMode {
return c.mode
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return c.mode
return ClientModeSentinel

Signed-off-by: Cheyu Wu <cheyu1220@gmail.com>
@CheyuWu CheyuWu force-pushed the feat/client/mode branch from 1ad61f9 to 6d9ed43 Compare March 6, 2025 16:37
@CheyuWu
Copy link
Copy Markdown
Contributor Author

CheyuWu commented Mar 6, 2025

Hi @rueian, I have solved those issues, PTAL

@rueian
Copy link
Copy Markdown
Collaborator

rueian commented Mar 6, 2025

Hi @rueian, I have solved those issues, PTAL

Hi @CheyuWu, LGTM! Could you add simple tests for these Mode() as well? I'd like to keep our test coverage from dropping.

Signed-off-by: Cheyu Wu <cheyu1220@gmail.com>
@CheyuWu
Copy link
Copy Markdown
Contributor Author

CheyuWu commented Mar 7, 2025

Hi @rueian, I have solved those issues, PTAL

Hi @CheyuWu, LGTM! Could you add simple tests for these Mode() as well? I'd like to keep our test coverage from dropping.

Hi @rueian, I have added the unit test. If there is any additional test that I should add, please let me know.

@rueian rueian merged commit 714ea64 into redis:main Mar 7, 2025
@rueian
Copy link
Copy Markdown
Collaborator

rueian commented Mar 7, 2025

Merged. Thanks @CheyuWu!

@FZambia
Copy link
Copy Markdown
Contributor

FZambia commented Mar 7, 2025

Many thanks for implementing this @CheyuWu @rueian ! ❤️

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.

A way to understand whether Client is a clusterClient

3 participants