Support for PubSub Lock on AWS Elasticache Valkey #10473
Conversation
There was a problem hiding this comment.
Please, add DCO into your commit message before we start reviewing: https://spring.io/blog/2025/01/06/hello-dco-goodbye-cla-simplifying-contributions-to-spring.
Thanks
Done! |
| return Boolean.TRUE.equals(RedisLockRegistry.this.redisTemplate.execute( | ||
| UNLINK_UNLOCK_REDIS_SCRIPT, Collections.singletonList(this.lockKey), | ||
| RedisLockRegistry.this.clientId, RedisLockRegistry.this.unLockChannelKey)); | ||
| RedisLockRegistry.this.clientId, RedisLockRegistry.this.unLockChannelKey + ":" + this.lockKey)); |
There was a problem hiding this comment.
I understand how this work from the pattern perspective.
But is it really OK to have so many channels for every single lock we are going to use in our application?
There was a problem hiding this comment.
Maybe not. However, I do not see any other way to ensure that all three redis calls in the UNLINK_UNLOCK_SCRIPT hash to the same shard if we are running on a system that requires it, like Valkey on AWS.
If the call to PUBLISH would not be in that script, that would make all calls in that script use the same KEYS[1] as the first argument to a redis call. But, I suppose is has to be in there.
If that were not necessary, the call to Publish could be in a separate script, but that might not be okay, for reasons I do not yet know.
There was a problem hiding this comment.
I did some research in the middle of typing out the last message.
As far as I do understand it, lots of channels are not an issue. This, as channels do not consume memory, only subscribers do, irrespective of pattern or exact match.
There was a problem hiding this comment.
On the other hand, extracting the PUBLISH call to a separate script might be okay, but it could happen that the application dies between the two script calls and that would make other subscribers wait for the remaining TTL of the lock.
On another note, my understanding is currently that any values used as a key of an operation should be included in the keys, rather than the args. Meaning either the current or my new unlockChannelKey should be passed as KEYS[2] rather than ARGV[2]. See https://docs.aws.amazon.com/AmazonElastiCache/latest/dg/WorkingWithRedis.html, bullet point "Use sharded pub/sub" or https://redis.io/docs/latest/develop/pubsub/#sharded-pubsub
There was a problem hiding this comment.
This looks more closer to what I won't worry about 😄 .
If you confirm that this works with Valkey, please, add your name to the @author list of the changed class.
Please, also rebase your branch (and re-push to PR) to the latest main as I've just pushed the fix Redis tests according to the latest Spring Data Redis.
Thanks
…ontrol cluster slot hashing As a consequence, change the ChannelTopic to a PatternTopic to keep receiving subscription events for PubSub lock Signed-off-by: Severin Kistler <[email protected]>
…f `ARGV[2]` as values used as keys should be passed as such Signed-off-by: Severin Kistler <[email protected]>
|
According to my tests against valkey this is working. |
This is an attempt to make issue #10471 work without support for
SPUBLISH.I append the
lockKeyto thechannelUnlockKey. This allows us to control the cluster slot cache this operation is hashed into, by including a hash tag ({...}) in thelockKey. Refer to the issue description for an explanation of why this matters.As a consequence, I had to change the
ChannelTopicto aPatternTopicto keep receiving subscription events for the PubSub lock.