-
Notifications
You must be signed in to change notification settings - Fork 58
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
Bump redis-rs + Route Function Stats to all nodes #2117
Conversation
There was a problem hiding this 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
Some(ResponsePolicy::Special) => { | ||
// Await all futures and collect results | ||
let results = future::try_join_all(requests).await?; | ||
let map_entries = self |
There was a problem hiding this comment.
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
ae4394b
to
e9645bd
Compare
There was a problem hiding this 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("protocol", [ProtocolVersion.RESP2, ProtocolVersion.RESP3]) | ||
async def test_function_stats_cluster(self, glide_client: GlideClusterClient): | ||
async def test_function_stats_running_script( |
There was a problem hiding this comment.
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 <[email protected]>
Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Co-authored-by: Yury-Fridlyand <[email protected]> Co-authored-by: Andrew Carbonetto <[email protected]> Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: Shoham Elias <[email protected]>
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