Skip to content

Conversation

cxljs
Copy link
Contributor

@cxljs cxljs commented Aug 17, 2025

Currently, when users use failover mode and want to implement read-write splitting, a close alternative is to use NewFailoverClusterClient with RouteRandom=true, but this is not the best solution.

Two simple solutions:

  1. Reuse FailoverOption.ReplicaOnly:
    When using NewFailoverClient(), ReplicaOnly=true means allowing all commands to be routed to replicas.
    When using NewFailoverClusterClient(), ReplicaOnly behaves the same as ClusterOption.ReadOnly.
  2. Add a new configuration entry called ReadOnly.

@ndyakov @htemelski-redis @elena-kolevska Which solution do you think is better, or do you have another alternative?

@ndyakov
Copy link
Member

ndyakov commented Aug 18, 2025

@cxljs I am not sure if ReadOnly will work correctly with failover cluster. Can we add couple of tests for it?

@cxljs
Copy link
Contributor Author

cxljs commented Aug 18, 2025

From the code, ReadOnly seems to work fine. I'll add tests later.

@cxljs cxljs force-pushed the failover-readonly branch from 82b3deb to 0ed1576 Compare August 18, 2025 09:53
@ndyakov
Copy link
Member

ndyakov commented Aug 28, 2025

@cxljs reminder for tests here, if we can merge this today we can release tomorrow / on monday

@cxljs
Copy link
Contributor Author

cxljs commented Aug 29, 2025

During my testing, I confirmed through the cluster logs that the ReadOnly option takes effect. However, the ClusterClient does not provide a corresponding API, so I couldn't write a test for it. Can I add a public API for test? Or do you have any idea? @ndyakov

@cxljs cxljs changed the title Fix the ReplicaOnly option does not take effect when using NewFailoverClusterClient Allow users to enable read-write splitting in failover mode. Aug 29, 2025
@cxljs
Copy link
Contributor Author

cxljs commented Aug 29, 2025

@ndyakov Sorry, I didn't explain the purpose of this PR in detail before. I've now added more information.

@ndyakov ndyakov merged commit 6bc7238 into redis:master Sep 1, 2025
20 checks passed
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.

2 participants