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.

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

Core: Add support for sending multi-slot JSON.MSET and JSON.MGET commands #2587

Merged
merged 6 commits into from
Nov 7, 2024

Conversation

barshaul
Copy link
Collaborator

@barshaul barshaul commented Nov 4, 2024

Description

This PR adds support for handling JSON.MSET and JSON.MGET commands with multi-slot keys, expanding the argument patterns that are currently supported. Prior to this PR, the system only supported two argument patterns for MGET and MSET: keys only, and key value pairs. This update introduces support for two additional patterns:

  1. Keys and Path: Relevant for the JSON.MGET command, which follows the format JSON.MGET key1 key2 key3 path.
  2. Key Path Value Triples: Relevant for the JSON.MSET command, which follows the format JSON.MSET key1 path1 value1 key2 path2 value2.

All patterns are now being represented with the MultiSlotArgPattern enum.

Multi-Slot Command Handling:

There are two main parts in handling multi-slot commands:

  1. Command Splitting:

    • The multi_shard function is responsible for splitting commands into sub-commands based on the slots each key belongs to. This PR enables the correct splitting of JSON.MGET and JSON.MSET commands based on the newly added argument patterns (KeysAndPath and KeyPathValueTriples).
  2. Response Aggregation:

    • There are two main response policies for aggregating responses from sub-commands:
      • Array Combination (default for commands like MGET and JSON.MGET): Combines all sub-command responses into a single array, maintaining the order of commands in the original request.
      • AllSucceeded (used for commands like MSET and JSON.MSET): Returns OK if all sub-commands succeed; otherwise, returns the first encountered error.

Although MSET and JSON.MSET currently don’t use combined array responses in Glide, this PR adds support for combining results into a single array for these commands too. This enhancement prepares for a future feature where responses can return an array of OK and Error results, allowing the operation to continue without failing entirely if a request for a single slot fails.

This extension enables enhanced multi-slot handling for JSON.MGET and JSON.MSET, improving the client's ability to manage complex, multi-slot commands and aggregate responses accurately.

Issue link

This Pull Request is linked to issue (URL): closes #2455

Checklist

Before submitting the PR make sure the following are checked:

  • [ X] This Pull Request is related to one issue.
  • [X ] Commit message has a detailed description of what changed and why.
  • [X ] Tests are added or updated.
  • [ X] CHANGELOG.md and documentation files are updated.
  • [ X] Destination branch is correct - main or release
  • [ X] Commits will be squashed upon merging.

@barshaul barshaul marked this pull request as ready for review November 4, 2024 09:23
@barshaul barshaul requested a review from a team as a code owner November 4, 2024 09:23
@Yury-Fridlyand Yury-Fridlyand added the Rust core redis-rs/glide-core matter label Nov 4, 2024
@Yury-Fridlyand
Copy link
Collaborator

Thanks

glide-core/redis-rs/redis/src/cluster_routing.rs Outdated Show resolved Hide resolved
/// Represents the pattern of argument structures in multi-slot commands,
/// defining how the arguments are organized in the command.
#[derive(Debug, Clone, PartialEq)]
pub enum MultiSlotArgPattern {
Copy link
Contributor

Choose a reason for hiding this comment

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

So the idea is to cover all options here? e.g. if I look at HSET, which has HSET KEY field value [field value..] are we going to create a new enumerator here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

HSET has a single key, so it wont be a multi-slot command. But, in general, yes - if there is going to be a command with a new pattern that we can support it in a multi-slot option, we will add it here

glide-core/redis-rs/redis/src/cluster_routing.rs Outdated Show resolved Hide resolved
glide-core/redis-rs/redis/src/cluster_routing.rs Outdated Show resolved Hide resolved
glide-core/redis-rs/redis/src/cluster_routing.rs Outdated Show resolved Hide resolved
glide-core/redis-rs/redis/src/cluster_routing.rs Outdated Show resolved Hide resolved
@barshaul barshaul force-pushed the json_mset_mget branch 2 times, most recently from a7a336f to 8a25a60 Compare November 5, 2024 17:00
@barshaul barshaul requested a review from eifrah-aws November 5, 2024 17:01
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

LGTM.
Can't post an approval, because I can't assess the rust code part.

go/api/commands.go Outdated Show resolved Hide resolved
node/src/BaseClient.ts Outdated Show resolved Hide resolved
glide-core/redis-rs/redis/src/cluster_routing.rs Outdated Show resolved Hide resolved
glide-core/redis-rs/redis/src/cluster_routing.rs Outdated Show resolved Hide resolved
glide-core/redis-rs/redis/src/cluster_routing.rs Outdated Show resolved Hide resolved
@barshaul barshaul mentioned this pull request Nov 6, 2024
6 tasks
…ands

Signed-off-by: barshaul <barshaul@amazon.com>
…NK, EXISTS, TOUCH, WATCH

Signed-off-by: barshaul <barshaul@amazon.com>
Signed-off-by: barshaul <barshaul@amazon.com>
Signed-off-by: barshaul <barshaul@amazon.com>
Signed-off-by: barshaul <barshaul@amazon.com>
Signed-off-by: barshaul <barshaul@amazon.com>
@barshaul barshaul merged commit 7c2c5e4 into valkey-io:release-1.2 Nov 7, 2024
55 checks passed
@barshaul barshaul deleted the json_mset_mget branch November 7, 2024 10:13
EdricCua added a commit to EdricCua/valkey-glide that referenced this pull request Nov 28, 2024
…ype Signed-off-by: EdricCua <ecuartero@google.com>
EdricCua added a commit to EdricCua/valkey-glide that referenced this pull request Nov 28, 2024
…ype.

Signed-off-by: EdricCua <ecuartero@google.com>
EdricCua added a commit to EdricCua/valkey-glide that referenced this pull request Dec 13, 2024
…ype.

Signed-off-by: EdricCua <ecuartero@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rust core redis-rs/glide-core matter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants