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

Add support for multi-shard commands. #900

Merged
merged 3 commits into from
Sep 12, 2023
Merged

Add support for multi-shard commands. #900

merged 3 commits into from
Sep 12, 2023

Conversation

nihohit
Copy link
Contributor

@nihohit nihohit commented Jul 23, 2023

This PR is based over #888 .

This allows the async cluster client to split keyed commands so that
keys will be grouped by the slot they belong in, and sent only to the
relevant shard.

@nihohit nihohit mentioned this pull request Jul 23, 2023
Merged
@nihohit nihohit force-pushed the multi-shard branch 4 times, most recently from a7a8cb6 to 302f2cc Compare July 23, 2023 08:41
@nihohit nihohit force-pushed the multi-shard branch 3 times, most recently from c979c81 to b21dac3 Compare August 16, 2023 19:24
@nihohit
Copy link
Contributor Author

nihohit commented Aug 16, 2023

@jaymell rebased

jaymell
jaymell approved these changes Sep 8, 2023
View reviewed changes
Copy link
Contributor

@jaymell jaymell left a comment

Choose a reason for hiding this comment

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

I think this looks good, but not gonna lie, trying to read through a few parts of this was pretty cumbersome. I'm not sure if it's possible to make things any more readable, so just asking for a couple of comments on the denser parts for the benefit of anyone who has to look through it again later.

redis/src/cluster_routing.rs Show resolved Hide resolved
redis/src/cluster_routing.rs Show resolved Hide resolved
redis/src/cluster_routing.rs Show resolved Hide resolved
@nihohit
Split routing info to single and multiple nodes.
408a7df
@nihohit
Add support for multi-shard commands.
02a88e4 
This allows the async cluster client to split keyed commands so that
keys will be grouped by the slot they belong in, and sent only to the
relevant shard.
@nihohit
Clarify combine_and_sort_array_results.
2032d0e 
Add documentation, add a test, and change `multi_shard` to use
0-indexing on the result.
@nihohit
Copy link
Contributor Author

nihohit commented Sep 9, 2023

@jaymell I added docs, a sample test, and slightly changed the implementation of multi_slot to return a 0-based indices. Hopefully that'll make the result more readable and understandable.

@jaymell
Copy link
Contributor

jaymell commented Sep 12, 2023

Thanks for adding the comments! As for the actual code changes, I'll leave it up to you. If you think the code was more robust prior to the latest changes, for feel free to revert them. Otherwise let's go ahead and merge!

@nihohit nihohit merged commit 6fa0e92 into redis-rs:main Sep 12, 2023
10 checks passed
@nihohit nihohit deleted the multi-shard branch September 12, 2023 06:04
@tsmt-iris
Copy link

Hey, does this mean that pipelines using key on different shards are working with async cluster?

@nihohit
Copy link
Contributor Author

nihohit commented Dec 19, 2023

Hey, does this mean that pipelines using key on different shards are working with async cluster?

no, it doesn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaymell jaymell jaymell 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
@nihohit @jaymell @tsmt-iris