Fix the watch command bugs for the cluster client

[supercaracal]

• The redis-cluster-client gem had had several bugs regarding the transaction feature. They has been resolved with the version 0.7.11.
  • Implementation for transaction redis-rb/redis-cluster-client#299
• The above release makes a portion of our test cases failure. So this pull request fixes them.
  • In the cluster client, a block is needed for the watch command because of the guarantee for a critical section with an optimistic locking. This behavior is different from the standalone client. So the cluster client overrides the watch method.

[supercaracal]

This permission may be needed. I don't know why it becomes needed since when. Some cases became failure in the ACL test.

[KJTsanaktsidis]

I'm not sure this is the right interface that redis-cluster-client/redis-clustering should be exposing.

The normal redis-rb client interface looks like this:

redis.watch('{key}1', '{key}2') do redis.get('{key}1') redis.get('{key}2') # Make some decision about what to do based on the value of these keys redis.multi do |tx| tx.set('{key}1', 'new_v1') tx.set('{key}2', 'new_v2') end end 

The differences are...

• watch does not actually yield anything (EDIT: it yields self but there are plenty of documented examples where the yielded value is ignored)
• A transaction is not actually started unless multi is called

Would you like me to put up a different pair of PR's (one here, one in redis-cluster-client) to try and show how I think we could implement it in a compatible way?

[supercaracal]

In my implementation, the block is finally passed to the redis-cluster-client gem. I think the redis-cluster-client gem should not be complex in excess due to the redis gem. I think the transaction feature is quite different between standalone and cluster. But of course, I think the other approaches are welcome.

[KJTsanaktsidis]

I think I can put most of the "hacking"/complexity in redis-clustering, but keep an interface in redis-cluster-client that actually makes sense in cluster mode.

Basically, redis-clustering is the "adapter" that takes the redis-cluster-client interface and makes it fit the redis-rb API pattern. That seems to make the most sense to me.

Let me try and get a couple of PR's up today :)

[KJTsanaktsidis]

@supercaracal what do you think about this approach? #1256 and redis-rb/redis-cluster-client#339

Basically, exposing the OptimisticLocking object from a dedicated #watch API on the redis-cluster-client side and then redis-rb makes that state implicit through the @active_watcher ivar.

[KJTsanaktsidis]

Sorry to be a pain here, but I really do want to ask whether this is actually the right interface @supercaracal @byroot

This PR defines an interface for watch where the client code looks like

r.watch('key1', 'key2') do |tx| tx.set 'key1', 'something in multi' tx.set 'key2', 'something else in multi' end 

but, ordinary not-clustered redis-rb watches look like

r.watch('key1', 'key2') do r.multi do |tx| tx.set 'key1', 'something in multi' tx.set 'key2', 'something else in multi' end end 

There are a few problems with what has been merged in my opinion:

• It is a different interface to not-clustered redis, so it makes it much harder for the same code to work in clustered and non-clustered redis
• there is no way in this interface to perform a WATCH, read a key, and then decide not to perform a transaction based on what was read and simply UNWATCH.
  *there is no way in this interface to perform a WATCH, read a key, and then decide to WATCH an additional key based on what was read

It requires a bit more support on the cluster-client side, but I believe the two PR's I mentioned above provide an alternative to this interface which resolves these problems and provides a better experience for users of both redis-rb/redis-cluster-client and also redis-cluster-client alone:

• Implement #watch and #multi specially for cluster-client #1256
• Add an explicit #watch method to RedisClient::Cluster redis-rb/redis-cluster-client#340

WDYT? Could this be revisited?

[byroot]

@supercaracal is the redis cluster maintainer, I'm merely merging because I'm not admin of the repo so I can't really officialize this.

I have 0 interest in Redis cluster and I trust @supercaracal to maintain it.

[supercaracal]

The redis gem of version 4.x or ealier didn't support the transaction feature in the cluster mode. ALso, The redis-clustering gem of version 5.1.0 or ealier didn't work correctly for the transaction feature and the watch command. So, I thought it should be fixed first.

As you said, it broke compatibility between standalone and cluster mode, but there is a trade-off for the simplicity. I said repeatedly, the redis-cluster-client gem shouldn't be affected excessively by the redis-clustering gem. Also, I think it shouldn't be messed up with its code and design.

If you desire the same interface between standalone and cluster in the transaction feature, I think it should be implemented by the redis-clustering gem or your own adapter. At least, I don't have a plan to change the interface of the redis-cluster-client gem related to the transaction feature except any bugs.

• Implementation for transaction redis-rb/redis-cluster-client#299

[KJTsanaktsidis]

I said repeatedly, the redis-cluster-client gem shouldn't be affected excessively by the redis-clustering gem. Also, I think it shouldn't be messed up with its code and design.

But it's not just about not being the same as redis-clustering. The redis-clustering design is the way it is because that's the shape it has to be in in order to support workflows like WATCH-then-UNWATCH or WATCH-then-WATCH-again like I said here:

• there is no way in this interface to perform a WATCH, read a key, and then decide not to perform a transaction based on what was read and simply UNWATCH.
• there is no way in this interface to perform a WATCH, read a key, and then decide to WATCH an additional key based on what was read

These are not esoteric use-cases, a primary purpose of Redis's WATCH API is to enable mixing application logic with Redis reads and then conditionally perform an atomic sequence of writes after that.

I think it should be implemented by the redis-clustering gem

But you closed this option off by designing #watch to work in redis-clustering this way rather than the way in #1256

or your own adapter

This isn't possible (without gratuitious amounts of send and instance_exec). We started going down this path by implementing #with, but then you started adding watch support into redis-cluster-client in a different way (with redis-rb/redis-cluster-client#333).

At least, I don't have a plan to change the interface of the redis-cluster-client gem related to the transaction feature except any bugs.

In my humble opinion... the client library of a popular open-source database should support the full range of operations that the underlying database supports. That's worth a small amount of internal complexity in the gem.

[supercaracal]

In the redis-cluster-client side, it's already the same interface between the redis-client gem and the redis-cluster-client gem. So I don't think any additional implementation is needed.

https://github.com/redis-rb/redis-client/blob/e79e9e52c247b403eb7bdff8ff87fe006477ac7a/lib/redis_client.rb#L442-L484