Support fan-out commands in cluster-async #843
Conversation
|
Hey, this is great! I've got a pending (unrelated) PR that touches a lot of the same code, which I'd like to get in first, purely for my own benefit 😛, if you don't mind. I will try to have it raised in the next few days for your review. |
|
Such be life 🤷 . I'd appreciate it if you could browse through the code and point out design choices that will clash with your change, or if you can integrate parts that are relevant to your PR, in order to reduce merge complexity further down the line |
|
I think we'll be ok. Our concerns are mostly orthogonal, though there is some overlap in code touched. For instance, I've ripped out the cluster async Overall I think this looks pretty good. I would appreciate it if you could add a bit more description of changes made/motivations for them in your PR description. |
|
@jaymell added more details to the description. |
|
Still not quite ready, but raised draft of #844 for reference. |
|
@jaymell rebased |
|
@jaymell rebased |
This makes the ergonomics of converting `RoutingInfo` into `Route` much better, and will help in places where we're going to use both.
Instead of returning a (String, RedisResult), cluster_async operations now returns (OperationTarget, RedisResult), where operation target disambiguates single node operations from fan-out operations. This is required, since fan-out operations can't return a single address, and so error handling has to know how to handle this. Moved the routing information from RequestInfo into the CmdArg variants. This is done because the two variants require different routing information - pipelines can only be routed to single nodes, while Cmd can also be fanned-out. Moved the structs required for getting connections into a ConnectionsAndMetadata struct, which is wrapped in an Arc. This allows us to moved the objects between functions without using self, which allows us to create futures that can run in parallel - in order to fan out concurrently, instead of serially like in the sync cluster. The Arc also ensures that clones are cheap, and we only clone/create expensive structs at the last possible moment.
This fixes 2 issues with the previous implementation of `route_pipeline` 1. if the first command didn't have specific slot routing then `iter.next().map(route_for_command)?;` caused the function to return `None`, even if later commands had specific routes. 2. If 2 commands had conflicting routes then the function would return `None`, instead of choosing one of the routes. A `None` return value meant that a connection would be chosen randomly. This change ensures that if there is at least a partially valid route to be chosen, the client will choose it.
The test has been run multiple times locally and succeeded.
|
Thanks!! |
Commands that should be routed to all nodes or all masters will now be routed correctly.
This Pr includes:
Routestruct intoRoutingInfoas theSpecificNodevariant, instead of theReplicaSlot/MasterSlotvariants. This was done because the currentRoutingInfo->Routemapping functions lost information fromRoutingInfoin variants that don't map to a specific route.(String, RedisResult<Response>), cluster_async operations now return(OperationTarget, RedisResult<Response>), where operation target disambiguates single node operations from fan-out operations. This is required, since fan-out operations can't return a single address, and so error handling has to know how to handle this.RequestInfointo theCmdArgvariants. This is done because the two variants require different routing information - pipelines can only be routed to single nodes, while Cmd can also be fanned-out.ConnectionsAndMetadatastruct, which is wrapped in anArc. This allows us to moved the objects between functions without usingself, which allows us to create futures that can run in parallel - in order to fan out concurrently, instead of serially like in the sync cluster. TheArcalso ensures that clones are cheap, and we only clone/create expensive structs at the last possible moment.