Skip to content

Conversation

LINKIWI
Copy link
Contributor

@LINKIWI LINKIWI commented Aug 16, 2025

Currently, SUBSCRIBE (and its sibling variant commands) establish connections only to masters, regardless of the ReadOnly option specified in client options.

This PR allows go-redis to respect the ReadOnly option for subscriptions in the pubsub client, by preferring slave nodes for new connections. Note that in both global and sharded pubsub modes, messages are replicated to slaves by Redis.

Note that newConn in the pubsub client is only used for creating connections for long-lived subscriptions, so it does not need to consider any per-command R/W-family properties as in the case for commands outside of the pubsub client.

I added a new unit test case for both global and sharded subscriptions with ReadOnly set to true, with an additional assertion on the value of pubsub{shard}_channels as returned by INFO to additionally verify that the subscription is served only by slave nodes.

@ndyakov
Copy link
Member

ndyakov commented Aug 16, 2025

Hello @LINKIWI, this is a nice addition, I am working on something that touches the pubsub at the moment and would like to hold merging your PR once I am done. Do you have a need for this / is is urgent to merge?

@LINKIWI
Copy link
Contributor Author

LINKIWI commented Aug 17, 2025

Hello @LINKIWI, this is a nice addition, I am working on something that touches the pubsub at the moment and would like to hold merging your PR once I am done. Do you have a need for this / is is urgent to merge?

Thanks for the review. No worries, it is not urgent; happy to hold for now and rebase on top of the in-progress refactor once that is in. Thanks!

@ndyakov ndyakov added wait Can’t be processed temporarily for other reasons feature labels Aug 18, 2025
Copy link
Member

@ndyakov ndyakov left a comment

Choose a reason for hiding this comment

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

This looks good and instead of waiting for my work to be merged, I would prefer to include it in a version prior to releasing what I am working on (since it will be in a beta).

Thank you @LINKIWI !

@ndyakov ndyakov removed the wait Can’t be processed temporarily for other reasons label Sep 2, 2025
@ndyakov ndyakov merged commit 10121e9 into redis:master Sep 2, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants