Load script on all master nodes in cluster

[MichaelKubovic]

Motivation:

Redis uses command SCRIPT LOAD to load script to a script cache. This command should be run on every master node in cluster, otherwise node executing the script with EVALSHA fails on nodes that didn't load the script. Currently with clustered client, all SCRIPT commands are executed on a random node. My change handles this sub-command differently.

I have also considered a solution to introduce composite keys in REDUCERS (command + subcommand) but thought it would be premature abstraction, so I just implemented this directly without touching REDUCERS.

[Ladicek]

Code LGTM, though it doesn't feel right to special case this particular command. But I don't have a better suggestion at the moment :-)

[MichaelKubovic]

An alternative suggestion would be to expose API on RedisClusterClient to send an arbitrary command to all slots and let the code in the user-land deal with script loading. But I see two downsides to it: a) it exposes a low-level methods which would better be encapsulated b) high-level RedisAPI::script() will still suffer in cluster env.

[Ladicek]

Oh I totally agree this should work out of the box, without reaching for any special API. We will need to do something similar to properly support Redis transactions in the cluster, so please don't take my comment above as a dismissal. It's just something we need to solve, one day. Not necessarily in this PR.

[MichaelKubovic]

We will need to do something similar to properly support Redis transactions in the cluster

We are running basic transactions just fine with a help of batch. You can't use RedisAPI methods unfortunately because they'd run each command on a different connection. The only thing that doesn't work with batching though is WATCH.

I could imagine a method on RedisClusterClient to create an object of RedisAPIImpl in a connection mode for a particular slot or just random node if not specified. Then you could use that instance to do transactions even without batch and with watch. But that's for another PR :)

Is there something I need to do besides waiting in order to get this reviewed by maintainers?

[Ladicek]

When I mentioned transactions, I was thinking of something like this: quarkusio/quarkus#32361 (comment)

[Ladicek]

LGTM otherwise.

[Ladicek]

I don't know why CI fails, it passed just fine in a PR of mine. @MichaelKubovic could you please rebase? That might help.

[MichaelKubovic]

There are no new commits in master, so rebase didn't produce anything new. I've pushed a dummy commit to re-trigger build.

[Ladicek]

You don't need to add an extra commit, you can just amend the commit on top with no changes -- that will change the commit date, which is enough.

But I see that didn't help. I don't know what's wrong :-/

[MichaelKubovic]

There was a commit in master that fixes the issue :) Rebased.

[MichaelKubovic]

That didn't help, so I tried to increase the container version. I run it locally with the updated version anyway + some more changes to the container setup because I can't use those ports on my MacOS.

[MichaelKubovic]

Another attempt, it seems SCRIPT commands must be run on master nodes only.

[MichaelKubovic]

Could I create a separate PR for 4.x branch to get changes in the forthcoming minor relase?

[Ladicek]

LGTM.

Backport to 4.x should be OK, thanks!

[Ladicek]

I'm just thinking, should we special-case some other SCRIPT ... command as well? Looking at https://redis.io/commands/?name=script, maybe SCRIPT FLUSH would deserve similar treatment to SCRIPT LOAD. I'm not sure about SCRIPT EXISTS. (And I don't think anyone would use SCRIPT KILL or SCRIPT DEBUG with the Vert.x Redis client.)

WDYT?

[MichaelKubovic]

Yes, I think FLUSH has similar semantics in clustered environment. I can add it. I'm not quite sure about the others to be honest. If LOAD and FLUSH keep scripts consistent across cluster , then EXIST should probably reduce with logical AND. So it would be a kind of EXISTS ON ALL NODES.

If I ever written a utility that enables DEBUG with RedisAPI, where I have no control over which node am I talking to, I would prefer all master nodes to be in a debug mode rather than a random node. Although both scenarios seem highly unlikely to me.

And finally KILL would stop execution for script that is currently running, but EVAL always runs it on a single node, so with respect to symmetry, I'd expect it to run specifically on the node that I need - which cannot be achieved with RedisAPI. On the other hand, KILL will only stop execution if no write has been done, making it safe enough to run across the whole of cluster - the risk layis in potentially stopping other scripts that may be running at the same time. But it's still more useful, than hitting a random node.

So my bottom line... I would treat all SCRIPT commands the same way, not just the LOAD, primarily because hitting a random node, in all scenarios is even less useful than running it everywhere.