Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cluster: Create read_from_replicas option #635

Merged
merged 5 commits into from Jul 8, 2022
Merged

Cluster: Create read_from_replicas option #635

merged 5 commits into from Jul 8, 2022

Conversation

utkarshgupta137
Copy link
Contributor

Currently, all cluster commands in readonly mode first go to the replica & then in case of errors, they're redirected to the primary.
Instead of the unnecessary replica call & then creating a connection, we can check if a command is readonly or not & then set it to the appropriate node.
This results in a benchmark improvement of ~80% for write commands in readonly mode.
IMO, the option should be renamed from readonly to read_from_replicas, but that would create a breaking change.

djc
djc reviewed Jun 20, 2022
View reviewed changes
Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

This sounds good, pretty simple!

src/cluster.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@utkarshgupta137
Copy link
Contributor Author

@djc What do you think about renaming the parameter to read_from_replicas (same name as in redis-py)? We can keep readonly as a deprecated option.

@djc
Copy link
Contributor

djc commented Jun 20, 2022

@djc What do you think about renaming the parameter to read_from_replicas (same name as in redis-py)? We can keep readonly as a deprecated option.

Sounds good to me, including the deprecation!

@utkarshgupta137 utkarshgupta137 mentioned this pull request Jun 24, 2022
Open
6 tasks
@utkarshgupta137
cluster: add read from replicas mode
ec2a3d1 
- send write queries to primaries & read queries to replicas in read_from_replicas mode
- deprecate the readonly param in favour of the new read_from_replicas param
djc
djc reviewed Jun 26, 2022
View reviewed changes
src/cluster_client.rs Outdated Show resolved Hide resolved
src/cluster_client.rs Outdated Show resolved Hide resolved
src/cluster.rs Outdated Show resolved Hide resolved
djc
djc approved these changes Jun 30, 2022
View reviewed changes
Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me!

@utkarshgupta137
Copy link
Contributor Author

@djc Can you merge this? I will then rebase #640.

@djc djc requested a review from jaymell July 5, 2022 09:47
@utkarshgupta137 utkarshgupta137 changed the title Cluster readonly writes Cluster: read_from_replicas. Jul 6, 2022
@utkarshgupta137 utkarshgupta137 changed the title Cluster: read_from_replicas. Cluster: Create read_from_replicas option Jul 6, 2022
jaymell
jaymell reviewed Jul 8, 2022
View reviewed changes
src/commands.rs Outdated Show resolved Hide resolved
@utkarshgupta137
Copy link
Contributor Author

@djc @jaymell The PR is failing for 7.0.0 nightly only, that too for test_object_commands test, which is a standalone redis test not a cluster test. It looks like a nightly issue, not an issue with this PR.

@jaymell jaymell merged commit 36fa7e3 into redis-rs:main Jul 8, 2022
@jaymell
Copy link
Contributor

jaymell commented Jul 8, 2022

Looks great, merged! I'm wondering if it's worth dynamically getting the readonly commands from COMMAND output rather than hard-coding but either way this is great.

@utkarshgupta137 utkarshgupta137 deleted the cluster_readonly_writes branch December 1, 2022 08:41
nonirosenfeldredis pushed a commit to nonirosenfeldredis/redis-rs that referenced this pull request Feb 13, 2023
@utkarshgupta137 @nonirosenfeldredis
Cluster: Create read_from_replicas option (redis-rs#635)
4833436 
Send write queries to primaries & read queries to replicas in `read_from_replicas` mode;
deprecate the `readonly` param in favour of the new `read_from_replicas` param.
nonirosenfeldredis pushed a commit to nonirosenfeldredis/redis-rs that referenced this pull request Feb 26, 2023
@utkarshgupta137 @nonirosenfeldredis
Cluster: Create read_from_replicas option (redis-rs#635)
8645b26 
Send write queries to primaries & read queries to replicas in `read_from_replicas` mode;
deprecate the `readonly` param in favour of the new `read_from_replicas` param.
nonirosenfeldredis pushed a commit to nonirosenfeldredis/redis-rs that referenced this pull request Feb 26, 2023
@utkarshgupta137 @nonirosenfeldredis
Cluster: Create read_from_replicas option (redis-rs#635)
1d0f609 
Send write queries to primaries & read queries to replicas in `read_from_replicas` mode;
deprecate the `readonly` param in favour of the new `read_from_replicas` param.
nonirosenfeldredis pushed a commit to nonirosenfeldredis/redis-rs that referenced this pull request Feb 26, 2023
@utkarshgupta137 @nonirosenfeldredis
Cluster: Create read_from_replicas option (redis-rs#635)
bb37725 
Send write queries to primaries & read queries to replicas in `read_from_replicas` mode;
deprecate the `readonly` param in favour of the new `read_from_replicas` param.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaymell jaymell jaymell left review comments

@djc djc djc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@utkarshgupta137 @djc @jaymell