redis-rb

redis-rb copied to clipboard

Ref: https://github.com/redis/redis-rb/issues/1070

Ok, let's start the work to see what kind of roadblocks we'll run into.

One idea I'm toying with right now, is that rather than delegate everything to redis-client which would require Redis 6+, we could use redis-client with theRESP2 protocol and keep all our type casting. The downside is that it doesn't allow to delete as much code, the upside is that we don't break people who are still on Redis 5, and we get the much better drivers of redis-client.

I'd suggest being more aggressive. Why not drop Redis 5? redis-rb 4.x supports Redis 5 just fine, people can continue using it if they don't want to upgrade for some reason. Redis 6.0 is from April 2020 and every major Redis provider supports 6.x.

Why not drop Redis 5?

The problem isn't so much Redis 5, but RESP2. Some people migrated to Redis 6 or 7, but used proxies such as https://github.com/envoyproxy/envoy/issues/17571 which don't yet support the newer protocol. That pains me but...

So if I can support RESP2 in redis-rb 5.0, I can remove all the deprecated stuff and allow these people to upgrade, and in a shorter time I can drop RESP2 support in 6.0.

That's the idea so far, we'll see how it goes. In https://github.com/casperisfine/redis-rb/pull/1 a surprising amount of tests pass with extremely little effort.

Quick status update.

• The main test suite pass.
• The hiredis suite passes as well (including SSL tests).
• The sentinel suite pass but is still super flaky, maybe a bit more than before.
• I still need to look at the cluster test suite, but it seems to work for the most part.

Ok, so another status update:

• sentinel test suite is still super flaky, there might be some legit error in there, worth investigating.
• cluster test suite: 412 runs, 1795 assertions, 6 failures, 7 errors,, https://github.com/casperisfine/redis-rb/pull/3

Outstanding issues with cluster:

• redis-cluster-client requires ruby 2.7, not sure if necessary or not, but that makes testing harder.
• There's a bunch of properties normally access on the config object that are broken (e.g. client.id). I need to iterate on all the node, that may require some small change in cluster-client.
• A handful of broken tests around transactions and pipelining, I'll have to dig into them.

Overall I'm quite tempted to extra Redis::Cluster to its own gem (or inside redis-cluster-client?). But not sure how much it would complicate testing. @supercaracal what do you think?

After sleeping on it, I think short term the best is to indeed split it in another gem, but keep it in the same repo so that it's easy to share code.

One issue though is that the gem name is already in use -_- https://rubygems.org/gems/redis-cluster

Ok so:

• The cluster suite now pass:
  • We have a regression on transactions (multi). Unless I missed something redis-cluster-client outright don't support them, we used to support some of them before. Not sure how big a deal it is.
• sentinel is still flaky I still need to debug, but I confirmed at least a part of the failures are happening consistently with specific seeds, so I think some tests put the sentinel group in a bad state and make later tests fail.
• I tested this branch against sidekiq and activesupport test suites, aside from some test only code they have to select the driver, they all pass.

I think I'll bump the version number and merge this this afternoon, further changes can happen on master. I've pushed a 4.x branch in case we want to backport some stuff to 4.x.

Oh, and jruby is flaky too now...

Aaaand it's green!

Further work will happen on master.

I’m sorry for the late reply. I agree with the decision.

@byroot I might create a new issue... and let me know if I should, but wanted to see if I was missing anything. We use clustering for our Rails Cache (using the official redis_cache_store adapter in rails), and it seems like this change makes it so there is no easy path forward for us to upgrade to redis > 5... Before you could just pass the cluster parameter to the redis_cache_store adapter and it would work as intended and now it doesn't seem like it can support using Redis::Cluster.

https://github.com/rails/rails/blob/6-0-stable/activesupport/lib/active_support/cache/redis_cache_store.rb#L116-L128

Maybe I should work with the rails project to get clustering support working over there.

Rails RedisCacheStore accept a Redis instance as argument:

ActiveSupport::Cache::RedisCacheStore.new(redis: Redis::Cluster.new(....))

or

config.cache_store = :redis_store, { redis: Redis::Cluster.new(....) }

NB: I didn't test, I may have got some of the syntax partially wrong, but the general idea works.

Oh I definitely missed that... I'll play around with it. Thanks a lot!

...it was even right there in the method I highlighted 😅