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

Plan for redis-rb 5.0 #1070

Closed
byroot opened this issue Feb 14, 2022 · 15 comments
Closed

Plan for redis-rb 5.0 #1070

byroot opened this issue Feb 14, 2022 · 15 comments

Comments

@byroot
Copy link
Collaborator

byroot commented Feb 14, 2022

A large part of redis-rb maintenance cost comes from having to implement each command Redis add.
A year after Redis 6.2 release, we're still missing a large part of the newly added commands,
and tengentially commands added by Redis modules are hard to use.

This comes down to the RESP2 protocol that requires to know how to parse each command output.

However Redis 6.0 introduced the RESP3 protocol, which is properly typed
allowing clients to not know anything about the commands they are sending, which means a drastically simpler and smaller implementation that don't need keeping up with commands added upstream.

So I'd like to split redis-rb in multiple gems (probably kept in the same git repository)

redis-client gem

What I'd like to do is to start a new simpler redis-client gem, that only support Redis 6.0+ and pretty much only offer
the following API:

redis = Redis::Client.new(...)

# sending command
redis.call('SMEMBERS', 'mykey', ...)

# pipelining
redis.pipeline do |pipe|
  pipe.call(...)
end

We may also need some API for dealing with subscriptions commands, but that's about it unless I'm missing something.

This simple client would:

  • Not support redis-cluster, it should be implemented as another gem, the vast majority of people don't need it.
  • No longer wrap calls with a mutex, it's a mis-feature of redis-rb, to share a client between theads use a thread pool.

Users should be encouraged to use redis-client directly.

I explored this avenue back in 2019.

redis-cluster-client gem

Clustering support adds lots of complexity, so I think it should be shipped separately for people that need it.

It should however be API compatible.

One difficulty with clustering is that the client must be able to extract the parts of the command that are keys.

Right now this is handled with lots of ad hoc code for each command, but we should instead load the list of command signatures from the redis server with COMMAND and extract the keys that way.

With the above redis-cluster-client should be able to be a drop-in replacement for redis-client

redis-rb 5.0

redis-rb should then be refactored to be a thin wrapper around redis-client allowing to keep backward compatibility for the most part. Users would still be encouraged to migrate to redis-client even though there is not plan to sunset the redis gem any time soon.

Since we wouldn't have to know about return types, we could also explore generating the methods from the COMMANDS output, as to simplify keeping up with Redis.

Timeline

I don't have a specific timeline yet.

Redis 6.0 will soon be the oldest supported version. I also see that Sidekiq plans to only support Redis 6+ soon, so 2022 is likely the good time for this to happen.

@mperham
Copy link
Contributor

mperham commented Feb 14, 2022

Yes, I’d love to switch to something like redis-client and lose the redundant mutex. I would also up the minimum Ruby version to 2.7 and clean up any legacy IO hacks.

Sidekiq 7.0 will match Rails defaults of Ruby 2.7+ and require Redis 6+.

@supercaracal
Copy link
Contributor

Current implementation of cluster mode already extracts keys by using COMMAND information. It might be easy to separate them.

@supercaracal
Copy link
Contributor

Current redis-rb has the following features. Are they should to be out of support or to be separate?

@casperisfine
Copy link

Are they should to be out of support or to be separate?

So, I don't have a specific answer to it right now, but my reasoning is: 95% of users should only need redis-client.

My hunch says both Sentinel and consistent hashing are only used by a small minority of users. So they should probably come in a well integrated extra gem.

We'll have to experiment a bit how we can make all these plug an play. Maybe some kind of "middleware system" might make sense.

@casperisfine
Copy link

So I'd like to split redis-rb in multiple gems (probably kept in the same git repository)

So I've put a bit more thought into it, and while in theory I think a mono-repo with both redis and redis-client would be best, in practice I'm still not admin of redis/redis-rb etc, which creates some complications, most notably I can't add regular contributors as owners (thinking @supercaracal here who's been pretty much single handedly maintaining the redis-cluster stuff).

So I'm very tempted to do this in a new repo.

@casperisfine
Copy link

Ok, so a quick update. The code can be seen at: https://github.com/redis-rb/redis-client

I believe it's now pretty much feature complete for "regular" redis (no cluster support). At this stage is mostly need to be tested in production like conditions to iron out potential bugs.

There's also an open question on wether it should provide pooling out of the box or not (redis-rb/redis-client#4).

@mperham are you interested in trying to convert Sidekiq to use redis-client? Your feedback would be appreciated. Alternatively I can try to do it myself and open a PR. The goal would be mostly to see if there's any issue with the library "ergonomics".

Also @supercaracal, let me know if you'd like to participate on the cluster support, I can add you as a collaborator to the redis-rb organization, otherwise I'll probably look at that in a few days. I'm still undecided wether it should be a separate gem or not, and separate repo or not, let me know if you have an opinion.

@mperham
Copy link
Contributor

mperham commented Mar 21, 2022

@casperisfine I love the idea and I would be happy to help with a prototype spike if you want to open an issue or PR against Sidekiq's 7-0 branch. This week is bad as I'm taking a small vacation but I can review and work on it next week.

@casperisfine
Copy link

@mperham sounds good, I'll take a stab at it tomorrow. Enjoy your vacation!

@supercaracal
Copy link
Contributor

@casperisfine I would definitely like to participate in the organization.

I think it would be better to separate gem and repository for cluster support. It seems that Redis cluster V2 will no longer need heavy implementation of the clients. So we can maybe remove cluster support in the future. I'd say that the redis-client should be kept thin.

redis/redis#8948

@casperisfine
Copy link

@supercaracal sounds good. I sent you an invite, feel free to create a redis-cluster-client repo whenever you want.

For now redis-client is still a bit of a moving target, so it's up to you wether you'd like to experiment now, or wait a bit more. Feel free to tag me on issues or PRs if you need any information.

@supercaracal
Copy link
Contributor

@casperisfine I understand. Thank you for your invitation.

casperisfine pushed a commit to casperisfine/sidekiq that referenced this issue Mar 22, 2022
As discussed in redis/redis-rb#1070 (comment)

Current shortcomings (redis-rb/redis-client#6):

  - No `namespace` support. This one is debatable.
  - No connection `url` support (this will be added very soon)
  - No `reconnect_attempts` support. I'd like to add it, but in a smarter way.
casperisfine pushed a commit to casperisfine/sidekiq that referenced this issue Mar 28, 2022
As discussed in redis/redis-rb#1070 (comment)

Current shortcomings (redis-rb/redis-client#6):

  - No `namespace` support. This one is debatable.
  - No connection `url` support (this will be added very soon)
  - No `reconnect_attempts` support. I'd like to add it, but in a smarter way.
casperisfine pushed a commit to casperisfine/sidekiq that referenced this issue Mar 28, 2022
As discussed in redis/redis-rb#1070 (comment)

Current shortcomings (redis-rb/redis-client#6):

  - No `namespace` support. This one is debatable.
  - No connection `url` support (this will be added very soon)
  - No `reconnect_attempts` support. I'd like to add it, but in a smarter way.
casperisfine pushed a commit to casperisfine/sidekiq that referenced this issue Mar 28, 2022
As discussed in redis/redis-rb#1070 (comment)

Current shortcomings (redis-rb/redis-client#6):

  - No `namespace` support. This one is debatable.
  - No connection `url` support (this will be added very soon)
  - No `reconnect_attempts` support. I'd like to add it, but in a smarter way.
casperisfine pushed a commit to casperisfine/sidekiq that referenced this issue Mar 30, 2022
As discussed in redis/redis-rb#1070 (comment)

Current shortcomings (redis-rb/redis-client#6):

  - No `namespace` support. This one is debatable.
  - No connection `url` support (this will be added very soon)
  - No `reconnect_attempts` support. I'd like to add it, but in a smarter way.
casperisfine pushed a commit to casperisfine/sidekiq that referenced this issue Apr 12, 2022
As discussed in redis/redis-rb#1070 (comment)

Current shortcomings (redis-rb/redis-client#6):

  - No `namespace` support. This one is debatable.
  - No connection `url` support (this will be added very soon)
  - No `reconnect_attempts` support. I'd like to add it, but in a smarter way.
casperisfine pushed a commit to casperisfine/sidekiq that referenced this issue Apr 12, 2022
As discussed in redis/redis-rb#1070 (comment)

Current shortcomings (redis-rb/redis-client#6):

  - No `namespace` support. This one is debatable.
  - No connection `url` support (this will be added very soon)
  - No `reconnect_attempts` support. I'd like to add it, but in a smarter way.
@casperisfine
Copy link

Quick update here since it has been a while.

redis-client is now feature complete enough to support Sidekiq use case. Also the retrofit effort in Sidekiq showed that rebuilding redis-rb on top of redis-client should be fairly easy: sidekiq/sidekiq#5298.

Since Redis 5.x is no longer supported (October 31, 2021), I'll likely soon start working on a new major version of the redis gem as planned.

However since it's quite a big breaking change, I'll likely cut a few more 4.x releases to ease the transition. The first 5.x might also be beta releases, we'll see.

@supercaracal
Copy link
Contributor

redis-cluster-client gem is here: https://github.com/redis-rb/redis-cluster-client

@casperisfine
Copy link

❤️

@byroot
Copy link
Collaborator Author

byroot commented Aug 17, 2022

Closing as most of this has now been merged. I will likely release a beta in a couple days.

awiddersheim added a commit to awiddersheim/redis-rb that referenced this issue Dec 16, 2023
This change removes references to their being a `mutex` protecting the
underlying Redis connection from concurrent access. This was removed in
`5.x`[1].

[1]: redis#1070
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

No branches or pull requests

4 participants