Skip to content
This repository has been archived by the owner on Jan 9, 2024. It is now read-only.

Support reading from replicas on pipeline executions #450

Merged
merged 1 commit into from
Apr 24, 2021

Conversation

FranGM
Copy link
Contributor

@FranGM FranGM commented Apr 20, 2021

This is the smallest possible change that should still satisfy #400

I've been testing it with our own client code and it seems to work as expected.

Something to note about this implementation is that it won't necessarily stick to the same replica for all read operations within the same shard. There are two reasons for this:

  • changing this behaviour would complicate the implementation and probably require making the connection pools pipeline-aware somehow as well, or provide a custom connection pool for this use case.
  • I actually think the behaviour as implemented is preferable as it will distribute the load across replicas, which is the original goal of this change anyway.

@@ -53,6 +53,50 @@
TimeoutError,
)

# Not complete, but covers the major ones
# https://redis.io/commands
READ_COMMANDS = frozenset([
Copy link
Contributor Author

Choose a reason for hiding this comment

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

since I had to move READ_COMMANDS to make it available to the pipeline object I ended up converting it into a frozenset (stolen from #406) since it does speed up lookups by an order of magnitude 😮

FranGM added a commit to FranGM/baseplate.py that referenced this pull request Apr 20, 2021
This will allow us to take advantage of `read_from_replicas` support on pipelines when Grokzen/redis-py-cluster#450 is merged
@Grokzen
Copy link
Owner

Grokzen commented Apr 20, 2021

@FranGM This looks awesome and a much smaller change to get the feature out quicker. Please just dig into it and really confirm it without a doubt that commands is sent out to the replica nodes and i will take this for a run myself and i hope i can get a minor release out within this work week as this should be much faster to verify and get out :)

@FranGM
Copy link
Contributor Author

FranGM commented Apr 20, 2021

Thanks for the quick reply!

I can do better than that actually :) I have it running in production right now. Here's a graph showing CPU usage on primary and its three replicas before and after the change

Screenshot_20210420-224506

@Grokzen
Copy link
Owner

Grokzen commented Apr 21, 2021

Good enough for me :) I aim for this weekend at the latest to go through it and get out 2.1.3 with this included

@Grokzen
Copy link
Owner

Grokzen commented Apr 24, 2021

@FranGM Tested it out locally and i also wrote a example script for the readonly pipeline part that i will commit up after this merge to make it more easily testable for anyone who want to try it out.

@Grokzen Grokzen merged commit 2a4c77d into Grokzen:master Apr 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants