Implement "pinning" cluster connection to a single node - RedisClient::Cluster#with

[KJTsanaktsidis]

Hello @supercaracal !

This is a followup to our discussion on #294. This implements a new method RedisClient::Cluster#with.

This method yields a proxy object which supports all the things RedisClient::Cluster would normally support, but wraps them in a check to see that there is no cross-slot access. I checked in some documentation which explains how to use it.

The options it takes are:

• key: (mandatory) The key to hash to determine what slot to lock the yielded proxy to. Just passing a hashtag by itself is OK here.
• on_primary: (defaults to true). Whether to use the primary or (possibly) a replica to run commands on. I didn't document or test this because I'm not actually sure it's a good idea; on_primary: true will definitely run commands on the primary, but on_primary: false would run commands according to the configured replica affinity; which might still be the primary if you're using e..g :primary_only or :random_with_primary. I can probably delete this for now if you don't think it'll be useful (for myself, we run with :primary_only affinity, so I don't need it).
• retry_count: (defaults to zero). Whether to retry the block in the event of node connectivity or slot re balancing issue.s Since retrying user-provided blocks is not guaranteed to be idempotent, I guess this should default to not.

This goes along with a branch I have ready to go for redis-clustering as well: https://github.com/redis/redis-rb/compare/zendesk:ktsanaktsidis/handle_clustering_with. Once we're happy with how this looks, I can open up a PR for that as well. The clustering side of this essentially wraps #with so that the yielded proxy also responds to the redis-rb DSL.

What I have NOT done anything about in this PR is the existing implementation of RedisClient::Cluster#multi. I think our options are:

• Keep it how it is and don't touch it
• Re-implement it on top of #with, by lazily waiting for the first command in the transaction, and then calling #with for that node (I'm not 100% sure this will work though - we might have to reach into the privates of redis-client to e.g. call #checkin and #checkout on pooled instances outside of a block-scope).
• Delete it

I don't know what your feelings on backwards compatibility for this gem is, but I think personally we should just delete it if that's acceptable. It's confusing to have multiple ways to do transactions in the gem, and I think all use-cases can be covered by the #with interface.

Anyway - keen to hear your thoughts, thanks again for working with me on this!

[supercaracal]

[IMO] It looks like this method has two responsibilities. Those are finding a node and yielding a block. I'd say that it would be better to dedicate only finding a node in this method and to call #try_delegate by RedisClient::Cluster side.

[KJTsanaktsidis]

That seems fair enough. I pushed up a new version without this method at all.

• I wrote a new method find_node_key_by_key (it's a bit wordy, but does what it says) and re-implemented find_node_key and find_primary_node_key in terms of that.
• RedisClient::Cluster calls @router.find_node_key_by_key, then @router.find_node, and finally @router.try_delegate itself.

[KJTsanaktsidis]

Uh, so sorry @supercaracal - when you ran the tests I realised I'd forgotten to push one of the files up! I've done that now, hopefully it makes more sense!

[supercaracal]

I'd say that we can more simplify it like the following fix:

• [DO NOT MERGE] An example implementation for transaction feature supercaracal/redis-cluster-client#15

The above example accepts some compromises.

• The #multi method requires a parameter, watch or key. It is diffreent from redis-client.
• Redirection while resharding support is not implementation yet.
• The transaction feature in Redis cluster is tricky and users of the feature must be few. So backward compatibility is not important so far.

It seems that Redis server throws an error when we execute commands to multi slots and discards them. So I think we don't necessarily have to validate keys before hand.

flushdb
+OK
multi
+OK
set key2 2
+QUEUED
set key1 1
-MOVED 9189 10.10.2.7:6379
exec
-EXECABORT Transaction discarded because of previous errors.
get key2
$-1
get key1
-MOVED 9189 10.10.2.7:6379

flushdb
+OK
multi
+OK
set key2 2
+QUEUED
set key3 3
+QUEUED
exec
-CROSSSLOT Keys in request don't hash to the same slot
get key2
$-1
get key3
$-1

What do you think?

[KJTsanaktsidis]

Thanks for your thoughts.

The #multi method requires a parameter, watch or key. It is diffreent from redis-client.

This I'm fine with - it's easy enough to implement in terms of #with in that case. I can make that change tomorrow.

Redirection while resharding support is not implementation yet.

In terms of the rest of the complexity in this PR, I would respectfully say that I think it's worth it, in my opinion. The complexity comes really in two main chunks:

• The implementation of RedisClient::Cluster::Command#extract_all_keys. This is a "leaf" method; it's a fair bit of code, but it's analogous to the extract_first_key method, and it has really clear input/output requirements (which are tested). So I don't think this imposes a huge burden
• The implementation of all the delegation and wrapping in RedisClient::Cluster::Pinning. This is more complex complexity (sorry, I didn't have a better word) because it really affects how the whole library behaves.

Doing the delegation instead of exposing the underlying RedisClient instances directly (which is what your proposed PR does) is important in my opinion though:

• It's needed to implement resharding support correctly. I think I've taken a halfway good stab at doing this; I'm sure there are bugs with it but I'll find them & work them out.
• Implementing the resharding is of course important because, if we can make code work instead of throw errors when doing a cluster scale-out, it's a better experience for users if their code works :)
• It's also important because if your application performs all of its redis work under transactions, your proposed PR would mean that if a resharding happens, the application would never actually make a call to update_cluster_info! and so would be permenantly broken until its restarted! Even if we don't retry the errors, handling the exceptions in RedisClient::Cluster::Router#try_delegate and try_send is important because it updates redis-cluster-client's understanding of the cluster topology.
• Finally, I don't think directly exposing RedisClient instances is good, because its API becomes your API. By wrapping the instance in these delegators before returning them, redis-cluster-client maintains control over what library users can and cannot do with the underlying RedisClient instances.

I understand that as an open-source maintainer, you don't want people to throw complex code at you and then walk away when they're done. It is not at all my intention to do that with this contribution! Myself and @learoyklinginsmith have been working for the last few months on upgrading redis-rb in our large monolith application at Zendesk, and we (and the people who do our jobs after we're gone) will have an ongoing interest in keeping this code working well. We're going to find more bugs in this stuff as we roll this out to thousands of instances in production, and we're going to fix them and send the patches back. And if other people find bugs in any of this code, we have a real interest in fixing them because they probably affect us too!

---

I guess the summary of this long message is: I know this code is complex, but it's complexity buys real features that we and I believe others will want (not throwing errors during resharding), and we plan to be actively engaged here to help you maintain it, rather than simply throwing code over the wall.

[KJTsanaktsidis]

@supercaracal just wondering if you'd had any further thoughts on this?

[supercaracal]

I apologize for my delayed response. Thank you for your feedbacks from real world. It means a lot.

Sorry for several bugs. I think they should be fixed. I feel easy to merge by seperated and fined pull requests as possible.

In #with and #multi methods case, I think client side doesn't have to validate the keys which can be validated by server side.

As you said, cluster clients should handle redirection. But I feel the way to validate all keys of all commands in client side is going to far from simple implementation.

I'd like to consider #with and #multi methods seperatedly. I think both can simply handle redirection by handling errors instead of proxying commands.

#with method is only able to call commands with single connection. Users of our client are responsible for their keys in commands if using the method. It is hard to distinct redirected keys either caller-side bugs or resharded. But I think #with method is what it is. I think the behavior of redirections would be better to be able to specify by an option of the method.

I'd like to refer the envoy implementation.

• https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/other_protocols/redis#transactions
• redis: added support for transactions (#22162) envoyproxy/envoy#22729

Our client aims being simple. I'd like to find compromise.

I'm sorry for my bad English. I'm not good at English.

[KJTsanaktsidis]

I apologize for my delayed response. Thank you for your feedbacks from real world. It means a lot.

No, thank you for maintaining this library, and thank you for your detailed feedback on this issue so far.

In #with and #multi methods case, I think client side doesn't have to validate the keys which can be validated by server side.

To be clear, let's take this block of code as an example.

client.with(key: '{slot1}') do |conn|
    conn.call('SET', '{slot2}key', 'value')
end

My current implementation in this PR will raise RedisClient::Cluster::Transaction::ConsistencyError with a message about the slots being different, and will not actually send the SET command to Redis. You are saying that the behaviour should be that the SET command is send to Redis, and it'll raise RedisClient::CommandError because the server will reject the request.

I prefer to do the check ahead of time here, and raise ConsistencyError, for one main reason: Many slots hash to the same node, and if you're unlucky, {slot1} and {slot2} might actually wind up on the same physical Redis node. In that case, this code will work, but if a resharding later happens and one of them gets migrated, this code will stop working. I would like to make it impossible to accidently write cross-slot-but-same-node accessing code like this.

However, we can always add this checking later on. So, let's initially merge this feature without proactively checking the slot, and we can perhaps revisit this discussion some time next year if we find e.g. that people are raising issues about this behaviour?

I think both can simply handle redirection by handling errors instead of proxying commands.

Again, to be sure I understand you, some code. Let's imagine that this slot has been newly resharded, but we don't know it yet.

client.with(key: '{slot}', retry_count: 1) do |conn|
  conn.call('SET', '{slot}key', 'value')
end

In my current implementation, the order of events is the following

1. The underlying call to RedisClient#call inside the implementation conn.call raises a RedisClient::CommandError with message "MOVED 12345 127.0.0.1:6380"
2. The code inside the proxy object that is conn handles the exception, and performs shard reassignment (by calling assign_redirection_node in the router). Then, the exception is re-raised
3. The exception bubbles out of conn.call
4. the code in RedisClient::Cluster#with catches the exception and retries the block (because retry_count is > 0)
5. The block is called again with a different (correct) conn
6. conn.call works this time.

What you are proposing is that the order of events should instead be:

1. The underlying call to RedisClient#call inside the implementation conn.call raises a RedisClient::CommandError with message "MOVED 12345 127.0.0.1:6380"
2. that exception bubbles out of conn.call
3. that exception gets caught by code inside the implementation of client.with
4. the code in RedisClient::Cluster#with catches the exception, performs shard reassignment (by calling assign_redirection_node in the router), and retries the block (because retry_count is > 0)
5. The block is called again with a different (correct) conn
6. conn.call works this time.

Your way doesn't require that conn is a proxy object; it can be the real RedisClient instance, whereas my way requires that conn is some kind of proxy so that we can catch RedisClient::CommandError before the user sees it.

One reason I think we might need the proxy implementation, though, is what happens if we have two different client instances, that point to totally different Redis clusters? What happens if we get a MOVED response here:

client1.with(key: '{slot1}', retry_count: 1) do |conn1|
  client2.with(key: '{slot2}', retry_count: 1) do |conn2|
    conn1.call('SET', '{slot1}key', 'value')
  end
end

The MOVED response is for client1's cluster, but client2's #with method is what catches the exception. We'd then wind up moving the slot on the wrong client instance. The benefit of the proxy object around conn is that we can catch the exceptions right there with the client that generated them, so we know for sure we're the cluster that the exception was for.

I'd like to refer the envoy implementation.

The envoy implementation seems to work by detecting transaction-starting commands (they mention MULTI, but we'd need to do WATCH as well), then waiting for the next command which specifies a key, and then sending commands to a connection based on that.

I can have a go at doing it that way, although I think it'll look similar to #295 (although I'm sure I can do a better job of it now). The only bad thing about this approach is that you can't call block-based methods on RedisClient because we don't know what client to use until midway through our block, so you wind up needing to do this: https://github.com/redis-rb/redis-cluster-client/pull/295/files#diff-9f0a4776f6f551ae5ad413e0ffde321d2f64f89481adc5ec5b9ace823b67da25R94-R101. IMHO that's not a huge problem though.

TL;DR - I'll have another go at that tomorrow and open a new PR.

I'm sorry for my bad English. I'm not good at English.

No need to be sorry - your english is great and way better than any other language I speak!

[supercaracal]

Thank you for your detailed description.

I hadn't been able to consider the nested calling with multiple client instances. Certainly, the corner case is hard to support by simplified implementation. But I feel that corner cases like that would be better to be able to handle by user side. I think it would be better to add a public method to client to be able to refresh the state of the cluster.

More safer is certainly better. But who most knows the implementation in the block is the user. I think we might as well leave some responsibility to up to user side.

I'll look into reference implementations in other languages.

[KJTsanaktsidis]

I'll look into reference implementations in other languages.

I had a look at some of them and now I'm just sad. There are allegedly five official clients: https://redis.io/resources/clients/

Python

This one's easy, they just don't support MULTI at all with redis cluster mode

https://github.com/redis/redis-py/blob/a1df1cf47808fd55150d4983dba3437b329f9d15/redis/cluster.py#L2259C1-L2262C73

    def multi(self):
        """ """
        raise RedisClusterException("method multi() is not implemented")

Java

Also very easy:

https://github.com/redis/jedis/blob/be088e30c0acf35b79c437bb0ef98f90188e8392/src/main/java/redis/clients/jedis/JedisCluster.java#L251-L254

  @Override
  public Transaction multi() {
    throw new UnsupportedOperationException();
  }

.NET

As far as I can tell, it doesn't really support cluster mode at all (there's a recently merged PR which purports to do so, but it doesn't look like it implements even the most basic routing support? redis/NRedisStack#170

Go

The Go client has two implementations, depending on whether you're using WATCH or not.

There's a method (c *ClusterClient) Watch(ctx context.Context, fn func(*Tx) error, keys ...string) (https://github.com/redis/go-redis/blob/a1093022305ba41faea800104418811d4a0befdf/osscluster.go#L1590). This method will

• Look at keys and decide what connection this is for
• Calls the single-node version of watch with the passed in fn: https://github.com/redis/go-redis/blob/a1093022305ba41faea800104418811d4a0befdf/osscluster.go#L1615C24-L1615C24
• That will make a transaction proxy with c.newTx(), and then issue WATCH *keys down that tx: https://github.com/redis/go-redis/blob/a1093022305ba41faea800104418811d4a0befdf/tx.go#L59
• And then calls return fn(tx).

This is actually vulnerable to the same problem I identified with the nested calls to with on different clients; if the error returned by fn belongs to a different cluster, it will still get handled by the innermost cluster and hidden from the outermost cluster.

In other respects, it kind of behaves like what you suggested, where the underlying single-node connection object is passed directly into the callback.

Nodejs

It seems like node.js does support transactions...

• You can call MULTI on a cluster client: https://github.com/redis/node-redis/blob/d6d2064c72b99d34fc88318f3979177e3c89acd4/packages/client/lib/cluster/index.ts#L137
• It looks at the first key on the first command to work out what node to send the transaction to: https://github.com/redis/node-redis/blob/d6d2064c72b99d34fc88318f3979177e3c89acd4/packages/client/lib/cluster/multi-command.ts#L114C3-L114C6

But it doesn't look like WATCH is supported.

[KJTsanaktsidis]

I guess of these, the Go one is clearly the best (and is the closest to what I'm proposing in this PR as well - although it doesn't proxy the underlying Redis connections, it just passes them straight through).

[KJTsanaktsidis]

I feel easy to merge by seperated and fined pull requests as possible.

Would you like me to open separate PR's for each of these commits?

• 4051d6a
• d82a13e
• b639c43
• 70c3056
• 97d0499

[supercaracal]

I appreciate that your detailed investigation of referencing implementations for other clients. Most of them look like there may be no elaborate implementation yet. It might be just because complicated use cases are a few.

Would you like me to open separate PR's for each of these commits?

Yes, I would. I'd say that separating like the following could become easy to discussion and merge.

• Adding #with implementation
• Fixing twice execution of block in #multi
• Fixing other bugs
• Refactoring
• Adding some test cases with combination of #with and #multi
• Adding a public method to be able to refresh cluster state if needed, for use cases such as the nested block pattern, to be able to handle errors and internal state by user themselves
• etc...