Cluster: problem when using Slaves for READ operations

[Adrien-P]

I'm using Redis in Cluster mode with ioredis.
I discovered the { readOnly: true } option which looks amazing.
However, I've got some new MOVED errors since using this option.
It is only happening on write operations (del, hincrby, etc.).
I looked at the code and this piece of code (in cluster.js) attracted my attention:

          if (_this.options.readOnly) {
            redis = _this.selectRandomNodeForSlot(targetSlot);
          } else {
            redis = _this.slots[targetSlot].masterNode;
          }

My guess: when the option readOnly is activated, it selects randomly one of the nodes that is reponsible of the targetSlot. In a setup where you've got a Slave for each Master, the selection is made between two nodes (one Master and its Slave). It means that the Read operations are randomly distributed between these two nodes, which is the objective (even though I originally thought it would only go on Slave nodes).
However, it seems that the same process is applied for Write operations. It creates an issue because if the Slave node is selected for this operation, it will return a MOVED error. Eventually, with a few retries there is a good chance that the Master node will be picked by the random selector. So the operation has a good chance of succeeding. However, it looks very inefficient and with a lot of operations you'd get some operations actually failing.
Please let me know if it's a bug or if I'm not using the library properly

[luin]

Hi @Adrien-P,
You should only send readonly commands via the clients that have readOnly option enabled. Maybe we should improve our documentation to precisely delineate this behaviour, or improve the code to check whether the command that being sent is a readonly command, and if not, we should send it to the master node directly.

[Adrien-P]

Hi @luin , thanks for the answer.
Actually both options make sense. For the moment I'll create a readOnly client as you suggested for my read operations. I assumed the second option was the one already in place, this is why I got mislead. I think this second option where you make a smart choice between Master/Slave depending if it's write/read would make things easier as you'd have only one redis client to use for every operation.

[luin]

Yes, I agree with you. I'd like to deprecate the readOnly option in v2.0, and add a new option scaleReads (since I'm not a native speaker, I'm open to suggestions for the name 😆 ), which will automatically send reads to all nodes and writes only to the master.

[Adrien-P]

Yes I agree the readOnly option is not the best name for this. Because eventually, you can make write operations with this option too. I guess that's directly inspired from the READONLY Redis command.
scaleReads is closer to what we want indeed. I think that's a good name as long as there is a bit of text around it to explain what it does exactly.
Perhaps another option could be:
readFrom that could take the values: master or slave or all
Personnaly, I'd be interested in being able to pick only Slave nodes for Read operations (and never the Master node)

[nichliu]

For customize lua function, how can we know read or write?, maybe we need another option when we define the lua function?

[shaharmor]

hey @luin, any estimate on when v2 will be out? Anything we can do to speed it up?

[luin]

@shaharmor v2 will be out within two months.

[shaharmor]

@luin any update on this? we could really use this for scaling our cluster.

[luin]

It's the top priority in my list, however I still haven't got time to implement it. A project of my company will switch to redis cluster soon (we have 60GB redis data) and we rely on ioredis heavily, so I'll definitely have this feature implemented before our migration.

[shaharmor]

Not sure if this has been though about or not, but we would very much like if it would be possible to send all reads only to slaves, and not mix with master (When there are slaves available obviously).

[luin]

Yes, I've considered this case and there will be an option for this.

[luin]

Closing in favour of #247.