search

Nordix / hiredis-cluster

C client library for Valkey/Redis Cluster. This project is used and sponsored by Ericsson. It is a fork of the now unmaintained hiredis-vip.
BSD 3-Clause "New" or "Revised" License
86 stars 42 forks source link

Update slotmap on error in ToNode (sync API) #185

Closed zuiderkwast closed 1 year ago

zuiderkwast commented 1 year ago

With this change, an iteration over the nodes with a ToNode command to each node can now include a slotmap update which restarts the iteration. I guess it can be a little inconvenient. Is there anything we can do about it? At the very least, it can be worth mentioning in the release notes, but maybe we can do something better?

Possibilities for the node iterator when slotmap changes:

WDYT?

zuiderkwast commented 1 year ago
  • Restart iterator when slotmap has been updated (current behaviour). The user may get the same node multiple times. It may be confusing and can cause some problem. Why was this behaviour chosen?

@bjosv you added this, right?

bjosv commented 1 year ago

Maybe we should add something about this new behaviour in the README? We have some mentions about the redirection case, so this might be good to have some notes about to. https://github.com/Nordix/hiredis-cluster/blob/4dc5564c4356551327f480ffca5318fe59a61c73/README.md?plain=1#L274-L277

bjosv commented 1 year ago
  • Restart iterator when slotmap has been updated (current behaviour). The user may get the same node multiple times. It may be confusing and can cause some problem. Why was this behaviour chosen?

If I remember correctly the aim was to be able to cover and iterate all node at least once mainly for commands that couldn't be sent via the slot-based routing. That's why it was added at the same time as ToNode() commands.

The alternative then was to stop/abort when the slotmap had changed, but I guess there wasn't a good way to know if the iteration was finished or not. The user would also have needed to reinit the iterator to make sure all nodes was covered. Maybe an API to check if the route_version was changed or not during iteration could have solved this, not sure if that was up..

  • Make the iterator resistant to slotmap changes somehow. We may need to use something other than a dict, let's say init iterator can retrieve a list of addresses (the keys in the dict) and store them in a separate list in the iterator. On slotmap update, we can modify this list in a way that causes the least surprise to the user.

I see in the PR that this was briefly discussed, but the simplistic way to be able to stack allocate, like a hiredis dict, was choosen.

zuiderkwast commented 1 year ago

OK, thanks, so shall we stick with the current solution then?

We have the event callback for updated slotmap now, which can also be used in addition to checking if iterator.route_version != cc->route_version as is done in the test case. Maybe I should mention this too in the README. I'll see what I can come up with.