Upgrade to RedisClient #122
Conversation
|
Hey @sirwolfgang, this looks nice! Just opened sirwolfgang#1 to have CI passing. Also moving to RedisClient will make it compatible just Redis 6.0+, so made an update in README.md too. |
Minium Redis version required for redis-client
Spec fix + Update README.md
|
@san983 Just merged it in. Thanks for the support! |
|
@leandromoreira Can you give this a look over? I think it should be good to go, and cut the new version. The redis-client native connection pooler stuff is compatible via |
|
I usually left the |
| def lock(resource, val, ttl, allow_new_lock) | ||
| recover_from_script_flush do | ||
| @redis.with { |conn| conn.evalsha Scripts::LOCK_SCRIPT_SHA, [resource], [val, ttl, allow_new_lock] } | ||
| @redis.call('EVALSHA', Scripts::LOCK_SCRIPT_SHA, 1, resource, val, ttl, allow_new_lock) |
There was a problem hiding this comment.
Trying to debug why are locks aren't being made successfully while testing upgrade from 1.3.2 to 2.0.0
I think behavior here changed since the change to call doesn't return anything and therefore servers.select { lock }.size never returns a lock as found
There was a problem hiding this comment.
confirmed that since I'm passing in a redis client with a connection pool, it would work as @redis.with { |conn| conn.call .... } but not @redis.call
| begin | ||
| yield | ||
| rescue Redis::CommandError => e | ||
| rescue RedisClient::CommandError => e |
There was a problem hiding this comment.
seems we probably want to rescue RedisClient::ConnectionError here redis-rb/redis-client#92 (comment)
noting https://github.com/redis/redis-rb/blob/v5.0.6/lib/redis/client.rb#L7-L19
5 participants
Closes #121
Proposed Changes:
Gemfile.lock, generally this should not be tracked in gems. If anything needs to be locked, it should be in thegemspecRedisClients native pooler; My understanding is this is what sidekiq 7 is doing, since it dropped support for Connection Pool; and both gems are written by mperham2.0.0since breaking change