Update slotmap on error in ToNode (sync API)

[zuiderkwast]

No description provided.

[zuiderkwast]

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:

• 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?
• Abort iterator when slotmap has been updated. The user may think all nodes have been covered, which is also a problem. If the user needs to send a command to all nodes, this will not work well.
• 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.

WDYT?

[bjosv]

Nice!

[zuiderkwast]

• 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]

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.

hiredis-cluster/README.md

Lines 274 to 277 in 4dc5564

	Commands will be sent to the cluster node that the client perceives handling the given key.
	If the cluster topology has changed the Redis node might respond with a redirection error
	which the client will handle, update its slotmap and resend the command to correct node.
	The reply will in this case arrive from the correct node.

[bjosv]

• 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]

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.

[bjosv]

Very nice.

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

If we find a better alternative we can handle that in another PR.
Maybe either add some helper function to the existing iterator or create a new UniqueNodeIterator or something..