Skip to content

Bump redis-rs + Route Function Stats to all nodes#2117

Merged
shohamazon merged 9 commits intovalkey-io:mainfrom
shohamazon:redis-rs
Aug 20, 2024
Merged

Bump redis-rs + Route Function Stats to all nodes#2117
shohamazon merged 9 commits intovalkey-io:mainfrom
shohamazon:redis-rs

Conversation

@shohamazon
Copy link
Copy Markdown
Collaborator

@shohamazon shohamazon commented Aug 11, 2024

Since we've modified the FUNCTION STATS command to return responses from all nodes (instead of only from primaries), the response structure for standalone setups will change. Instead of returning a single response from a random node, it will now return a map of responses, where each key is the node's address and the value is the same node's response. Even in standalone mode.

Reason for the Change:
The main reason for this change is that a function or script could be running on a replica, and the previous FUNCTION STATS command wouldn't detect it, resulting in an incomplete response.

amazon-contributing/redis-rs#181

@shohamazon shohamazon requested a review from a team as a code owner August 11, 2024 08:21
@shohamazon shohamazon requested a review from barshaul August 11, 2024 17:32
Copy link
Copy Markdown
Collaborator

@barshaul barshaul left a comment

Choose a reason for hiding this comment

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

First of all - why should we have different response policy between standalone and cluster mode?

Second - this whole change shouldn't be under value conversion, but under standalone_client.rs::send_request_to_all_nodes. We already have response policy that matches this case if we decide to aggregate the responses: FirstSucceededNonEmptyOrAllEmpty.

But we should decide whats our approach here and we probably would like to keep unified experience over standalone and cluster mode. Is it possible in standalone that a script will run both on the primary and on a replica? or on multiple replicas? seems to me like we're going to loose information if we'll do this aggregation

@shohamazon shohamazon changed the title Bump redis-rs Bump redis-rs + Route Function Stats to all nodes Aug 14, 2024
Some(ResponsePolicy::Special) => {
// Await all futures and collect results
let results = future::try_join_all(requests).await?;
let map_entries = self
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

rename map_entries to a more informative name

@shohamazon shohamazon added python 🐍 Python wrapper node 🐢 Node.js wrapper java ☕ issues and fixes related to the java client breaking 💔 breaking changes labels Aug 15, 2024
@shohamazon shohamazon force-pushed the redis-rs branch 3 times, most recently from ae4394b to e9645bd Compare August 15, 2024 14:54
@shohamazon shohamazon self-assigned this Aug 15, 2024
@shohamazon shohamazon added the Core changes 🪐 Used to label a PR as PR with significant changes that should trigger a full matrix tests. label Aug 15, 2024
Copy link
Copy Markdown

@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.

Please update routing info for FUNCTION KILL in all clients

@pytest.mark.parametrize("cluster_mode", [False, True])
@pytest.mark.parametrize("protocol", [ProtocolVersion.RESP2, ProtocolVersion.RESP3])
async def test_function_stats_cluster(self, glide_client: GlideClusterClient):
async def test_function_stats_running_script(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

<3
Is it possible to do the same test in node and java?
I can help you with java part if needed.

Signed-off-by: Shoham Elias <shohame@amazon.com>
shohamazon and others added 8 commits August 20, 2024 07:20
Signed-off-by: Shoham Elias <shohame@amazon.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Co-authored-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Co-authored-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
Signed-off-by: Shoham Elias <116083498+shohamazon@users.noreply.github.com>
Signed-off-by: Shoham Elias <shohame@amazon.com>
Signed-off-by: Shoham Elias <shohame@amazon.com>
Signed-off-by: Shoham Elias <shohame@amazon.com>
Signed-off-by: Shoham Elias <shohame@amazon.com>
Signed-off-by: Shoham Elias <shohame@amazon.com>
@shohamazon shohamazon merged commit 45001ed into valkey-io:main Aug 20, 2024
@shohamazon shohamazon deleted the redis-rs branch August 20, 2024 08:43
@tjzhang-BQ tjzhang-BQ mentioned this pull request Apr 8, 2025
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking 💔 breaking changes Core changes 🪐 Used to label a PR as PR with significant changes that should trigger a full matrix tests. java ☕ issues and fixes related to the java client node 🐢 Node.js wrapper python 🐍 Python wrapper

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants