Execute multi-node requests using try_request. #14
Conversation
nihohit
changed the title
func from CmdArg::Cmd/Pipeline.This means multi node operations now get error handling, and can trigger MOVED, etc. Additionally, func was removed from CmdArg::Cmd/Pipeline. This removes the need to allocate a box on every request, and slightly reduces code complexity.
nihohit
force-pushed
the
try-request-multi-node
branch
from
827b3c6
to
e24f78b
Compare
redis/src/cluster_async/mod.rs
Outdated
| let result = conn.req_packed_command(&cmd).await.map(Response::Single); | ||
| return (addr.into(), result); | ||
| } | ||
| None |
There was a problem hiding this comment.
so, if 'asking', we get a random node? why?
There was a problem hiding this comment.
If we already have the connection, why won't we just do something like (in 723)
let mut conn = conn.await;
if asking {
let _ = conn.req_packed_command(&crate::cmd::cmd("ASKING")).await;
}
let result = conn.req_packed_command(&cmd).await.map(Response::Single);
return (addr.into(), result);
Is it even possible that we'll get a CommandRouting::Connection, and asking == true?
There was a problem hiding this comment.
so, if 'asking', we get a random node? why?
No, if asking then we'll get the redirected connection when calling get_connection.
If we already have the connection, why won't we just do something like
Because then we'd use the old connection instead of getting the redirected connection.
| @@ -699,20 +711,30 @@ where | |||
|
|
|||
| async fn try_cmd_request( | |||
There was a problem hiding this comment.
This function is a bit hard to understand. can you add some comments, e.g., in the first part - single node request without routing, second - multi node request, third - single node request with routing. something that will explain it a bit more, I got lost between the CommandRouting, routing, route_option, RoutingInfo...
There was a problem hiding this comment.
added some comments and restructured the function, please tell me if they are sufficient.
| @@ -454,12 +449,12 @@ where | |||
| let inner = Arc::new(InnerCore { | |||
| conn_lock: RwLock::new((connections, Default::default())), | |||
| cluster_params, | |||
| pending_requests: Mutex::new(Vec::new()), | |||
There was a problem hiding this comment.
otherwise it won't be accessible from within execute_on_multiple_nodes, and you won't be able to add the new requests to the queue.
|
@barshaul round |
This means multi node operations now get error handling, and can trigger MOVED, etc.
Additionally,
funcwas removed fromCmdArg::Cmd/Pipeline. This removes the needto allocate a box on every request, and reduces code complexity by eliminating an
unnecessary generic type.