Add key-based primary-client lookup to the cluster API - #175
Conversation
Add ered_cluster:get_client/2 to return the primary client responsible for a key, and get_clients/2 to resolve a list of keys while preserving input order. Reuse the slot-to-client lookup for normal command routing and add cluster coverage against CLUSTER SLOTS.
| command/3, command/4, command_async/4, command_async/5, | ||
| command_all/2, command_all/3, | ||
| get_clients/1, | ||
| get_clients/1, get_client/2, get_clients/2, |
There was a problem hiding this comment.
Another naming option here could be get_client_by_key/get_clients_by_keys 🤔 open to ideas!
There was a problem hiding this comment.
Yeah, maybe get_client_by_key/get_clients_by_keys are better. The get_clients/1 is not a perfect name either. Before we reach 1.0, we should revise the names of these less common functions (get_clients, get_addr_to_client_map, etc.).
Should the name reflect that this is only about the primaries? E.g. primary_by_key/primaries_by_keys? Would you ever want to get replicas by keys too?
Btw, what exactly do you want to use the client PIDs for? If you want to use it to batch cluster commands per node, when you get the client instances and call ered:command with each of them, they are not cluster-aware and don't handle redirects.
There was a problem hiding this comment.
Should the name reflect that this is only about the primaries? E.g. primary_by_key/primaries_by_keys? Would you ever want to get replicas by keys too?
We do not currently need to select replicas by key, so I agree the API should say that it returns primaries. I’ll rename these to primary_by_key/2 and primaries_by_keys/2.
Btw, what exactly do you want to use the client PIDs for?
In our use case, the returned client PID is only used as a grouping key; we do not issue commands directly to it with ered:command. We are not splitting multi-key commands, and the application remains responsible for the resulting non-atomic behavior.
|
After some discussion with other people involved, I think the better approach is that ered itself should handle pipelines with commands to multiple nodes. A pipeline is not atomic, so each command can return a redirect and ered should handle each redirected command separately. Only MULTI-EXEC transactions need to be considered an atomic unit and be redirected as a whole. The replies after redirect should then be combined into a list of replies in the same order. This issue contains some information: I believe [EDIT] To clarify, what we could do is to provide a As it's implemented currently, a pipeline (with an explicit Key provided for routing) is supposed to be for a single slot and it gets redirected as a whole, so even if you group the commands per primary node, it's not working correctly as you would need it to work (in the corner case when some slot is migrated). For that, you would need to group the per slot instead. |
|
Thanks @zuiderkwast, that makes sense (also apologies for the delay replying here). Yeah, grouping by primary is not sufficient because a primary owns multiple slots, and a pipeline that is initially routed to it can contain commands whose slots have moved independently. Retrying that pipeline as a unit would not be correct. Our current internal implementation is just a workaround, rather than an indication that ered should expose primary-client lookup as its intended batching API. We also use
A keyless Given that direction, I completely understand if you would prefer not to expose the primary-by-key helpers. Would you rather we close this PR, or revise its README/API framing so it does not encourage clients to group pipelines using these functions? |
|
It's great that we're alinged about the long-term direction.
I don't really know yet. We can leave it open, for the discussion. Then we can perhaps do some prototyping and see where it gets us. What do we actually need for supporting mixed slot pipelines?
|
This PR proposes (see #173) two new helper functions to the cluster API:
primary_by_key/2returns the primary client responsible for a key.primaries_by_keys/2returns the primary client for each supplied key, in input order. These can be used to group work by cluster node before sending commands directly to the selected clients.Both functions return
{error, unmapped_slot}when no primary is mapped for a key’s slot.The existing command-routing path now reuses the same slot-to-client lookup, keeping unmapped-slot behavior consistent.
Please let me know if the naming makes sense.
My only fear is that in the future you may have other plans forget_client/1/get_clients/2names but at the same time this lines up nicely with the existingget_clients/1.