Plan for switching to RedisClient

[san983]

Hi! Is there any plan or roadmap to move to https://github.com/redis-rb/redis-client that I can contribute to?

[leandromoreira]

I think it already works, have you tried the following code snippet replacing Redis by RedisClient? they might work under the same interface/api.

servers = [ 'redis://localhost:6379', Redis.new(:url => 'redis://someotherhost:6379') ]
redlock = Redlock::Client.new(servers)

[san983]

Thanks @leandromoreira. So, the interface will need some adjustments, AFAIS, "only in" ./lib/redlock/client.rb. I'm showing there have been some work on that file lately, so not sure how it would be the right way to make in compatible for both Redis and RedisClient gems

      def lock(resource, val, ttl, allow_new_lock)
        recover_from_script_flush do
-          @redis.with { |conn| conn.evalsha Scripts::LOCK_SCRIPT_SHA, keys: [resource], argv: [val, ttl, allow_new_lock] }
+          @redis.call('EVALSHA', Scripts::LOCK_SCRIPT_SHA, 1, resource, val, ttl, allow_new_lock)
        end
      end

      def unlock(resource, val)
        recover_from_script_flush do
-          @redis.with { |conn| conn.evalsha Scripts::UNLOCK_SCRIPT_SHA, keys: [resource], argv: [val, ttl] }
+          @redis.call('EVALSHA', Scripts::UNLOCK_SCRIPT_SHA, 1, resource, val)
        end
      rescue
        # Nothing to do, unlocking is just a best-effort attempt.
      end

      def get_remaining_ttl(resource)
        recover_from_script_flush do
-          @redis.with { |conn| conn.evalsha Scripts::PTTL_SCRIPT_SHA, keys: [resource] }
+          @redis.call('EVALSHA', Scripts::PTTL_SCRIPT_SHA, 1, resource)
        end
      rescue Redis::BaseConnectionError
        nil
      end

[sirwolfgang]

Other gems, including sidekiq, have moved over; So this is becoming out of sync

[leandromoreira]

Thanks @san983 and @sirwolfgang go ahead and implement that! I'm glad you're offering to do that. Maybe we could introduce a new wrapper client abstraction interface with the default implementation being the RedisClientWrapper.

    class RedisClientWrapper
       def lock_call(resource, val, ttl, allow_new_lock)
          recover_from_script_flush do
             @redis_client.call('EVALSHA', Scripts::LOCK_SCRIPT_SHA, 1, resource, val, ttl, allow_new_lock)
          end
       end
       # ...
    end

    class RedisWrapper
       def lock_call(redis_client, resource, val, ttl, allow_new_lock)
          recover_from_script_flush do
              @redis_client.with { |conn| conn.evalsha Scripts::UNLOCK_SCRIPT_SHA, keys: [resource], argv: [val, ttl] }
          end
       end
       # ...
    end
    
    # ... and then replace the places where it's should rely on redis client
    # and somehow offer a way for the user to pass their own redis client wrapper
    
      def lock(resource, val, ttl, allow_new_lock)
           @redlock_wrapper.lock_call(resource, val, ttl, allow_new_lock)
      end

But maybe this effort is too much and not so useful, what do you think?

[sirwolfgang]

Judging from how quickly people are migrating, I think the general consensus is that supporting both isn't necessary. It's likely prudent to just cut a breaking change, with the client updated.

[sirwolfgang]

@leandromoreira I just pushed a pass at setting up a PR for this, as version 2.0.0.

I do think some consideration needs to be made around the initialization interface. Based on upgrading to sidekiq 7, I suspect that we want to actually drop connection_pool support, and default to using the built in pooler of RedisClient.

This I think makes the two interfaces of either providing a premade RedisClient pooling-instance or having Redlock initialize on from the connection string the important ones to support.

[sirwolfgang]

The built in pooler is compatible with the connection_pool interface, so switching over and allowing someone to bring either types of pools works as-is. I think #122 is ready.

[leandromoreira]

Thank @sirwolfgang I'll look at right now!