Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement RedisClient::Cluster#with (again!) #311

Merged
merged 3 commits into from
Jan 22, 2024

Conversation

KJTsanaktsidis
Copy link
Contributor

This PR actually implements the #with method for checking out a connection directly to a particular Redis node. This allows you to perform transactional commands directly against the underlying RedisClient instance, so #watch, #multi, etc all work.

This does NOT yet implement any kind of validation for the commands issued. If you issue commands involving keys not owned by the node, you will receive Redis::CommandError's. A future PR will add client-side validation to hopefully make it possible to catch problems related to transaction misuse earlier in development. There's a pending test for this.

This PR also does not yet implement RedisClient::Cluster#multi in terms of #with. That will also come in a future PR.

This is needed so we can write shorter timeouts in for some tests which
will otherwise sleep for ~10 seconds waiting for timeouts to elsapse!
@KJTsanaktsidis KJTsanaktsidis force-pushed the ktsanaktsidis/impl_with_again branch from 3b5adf4 to 02acb9e Compare January 16, 2024 04:30
@supercaracal supercaracal self-requested a review January 16, 2024 04:58
@@ -93,6 +93,17 @@ def multi(watch: nil, &block)
::RedisClient::Cluster::Transaction.new(@router, @command_builder).execute(watch: watch, &block)
end

def with(slot_key:, write: true, retry_count: 0, &block)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering the naming of slot_key is general for Redis cluster users. Although it may be a bit redundant, would key_for_slot be easier to understand?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I bounced through a few options with this:

  • key: - This sounds nice, and sometimes you really will pass a key here (if you're doing MULTI on a single key). But, you can and normally will pass things which aren't even real keys here - i.e. just the bare hashtag like {foo}.
  • slot: - This is what the parameter is for - it calculates the slot based of what you pass in here - but it kind of implies that you actually pass a slot number in here, rather than a key (we could make it work by passing in a slot number too, but i don't see the use case - 99.999% of the time it'll be a hashtag you pass in).
  • hashtag: - this parameter would be "foo" instead of "{foo}". It has the right name, but if you're doing single-key operations you might not actually have a hashtag.
  • slot_key: - This is where I ended up, it has what it is ("key") and what it's for ("slot") in the name. It's a bit verbose though.
  • key_for_slot: - This is probably even more clear but also even more verbose than slot_key.
  • Another option is to make it slot:, have it take a number, and also expose a slot_for(key) method, so you'd call it as client.with(slot: client.slot_for(key)) do |conn|. This feels a bit overly complicated though because i can't think of a use-case for actually passsing a slot number in there that didn't come from hashing a key.

I don't mind too much which one we choose - they all have upsides and downsides. Happy for you to pick something off this list and I'll change the implementation to that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OR - how about we do both key: and hashtag:?

  • You can pass key:, and we'll hash it and use that slot. This will work for single keys and keys with hashtags in them
  • You can pass hashtag:, and we'll raise an exception if it doesn't match {...}. Then, we'll hash it and use that slot like key.

So you can write client.with(hashtag: '{foo}') do |conn| normally, but if you want to do single key operations, you can do client.with(key: 'fookey') do |conn|. It's slightly redundant because technically key can do everything that hashtag does, but using hashtag: makes it much clearer when reading the code that the block will operate on multiple keys with that hashtag.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I'd like to upvote your idea.

@KJTsanaktsidis KJTsanaktsidis force-pushed the ktsanaktsidis/impl_with_again branch 2 times, most recently from 81f9664 to d50fdd5 Compare January 21, 2024 22:48
KJ Tsanaktsidis added 2 commits January 22, 2024 09:57
This PR actually implements the #with method for checking out a
connection directly to a particular Redis node. This allows you to
perform transactional commands directly against the underlying
`RedisClient` instance, so `#watch`, `#multi`, etc all work.

This does NOT yet implement any kind of validation for the commands
issued. If you issue commands involving keys not owned by the node, you
will receive `Redis::CommandError`'s. A future PR will add client-side
validation to hopefully make it possible to catch problems related to
transaction misuse earlier in development. There's a pending test for
this.

This PR also does not yet implement `RedisClient::Cluster#multi` in
terms of `#with`. That will also come in a future PR.
We have lots of tests! That's a good thing!
@KJTsanaktsidis KJTsanaktsidis force-pushed the ktsanaktsidis/impl_with_again branch from d50fdd5 to 3586fec Compare January 21, 2024 22:57
@KJTsanaktsidis
Copy link
Contributor Author

@supercaracal This should be good to merge now hopefully! 🤞

@supercaracal supercaracal merged commit c5cf8e3 into redis-rb:master Jan 22, 2024
25 checks passed
@supercaracal
Copy link
Member

Thank you!

@KJTsanaktsidis KJTsanaktsidis deleted the ktsanaktsidis/impl_with_again branch January 22, 2024 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants