Use SET NX/EX for new master locking approach #734
Conversation
There was a problem hiding this comment.
I'm all for simplifying calls where appropriate.
In this case though, any user who previously qualified for the resilient lock (because the redis they were connected to was >= 2.5) will get "downgraded" to the basic lock when they upgrade this plugin.
I would prefer an implementation based on SET NX/EX to exist along side the existing lua-based resilient lock index, not to replace it.
carsonreinke
force-pushed
the
master-lock-woes
branch
from
9ed46cc
to
96930f2
Compare
| module Resque | ||
| module Scheduler | ||
| module Lock | ||
| class ResilientModern < Resilient |
There was a problem hiding this comment.
I couldn't come up with a better name
There was a problem hiding this comment.
What does inheriting from Resilient give us?
I'd prefer inheriting from base and add a implementation for locked?. This would make it easier to remove the Resilient (and Basic) lock implementations in the future, which I can't imagine we wouldn't do now that we have this awesome new lock implementation (and redis 2.x is very old at this point).
There was a problem hiding this comment.
@iloveitaly The Resilient implementation offers the locked? method which retrieves and renews the lock. So, it looks like in Redis 6.2 a similar command was added.
@yaauie suggested I retain the existing implementation to avoid users downgrading to a worse master lock implementation if they upgraded resque-scheduler.
One possibility could be switching the Redis requirement to 6.2 and completely removing the inheritance and Lua script implementation.
carsonreinke
force-pushed
the
master-lock-woes
branch
from
96930f2
to
284beca
Compare
carsonreinke
changed the title
| @@ -97,7 +104,7 @@ def master_lock_key | |||
| end | |||
|
|
|||
| def redis_master_version | |||
| Resque.data_store.redis.info['redis_version'].to_f | |||
| Gem::Version.new(Resque.data_store.redis.info['redis_version']) | |||
There was a problem hiding this comment.
Why the change to using Gem::Version here?
There was a problem hiding this comment.
@iloveitaly this offers "native" version number parsing/comparison which was needed to see when the Redis feature was available. Specifically, the patch level comparison.
| module Resque | ||
| module Scheduler | ||
| module Lock | ||
| class ResilientModern < Resilient |
There was a problem hiding this comment.
What does inheriting from Resilient give us?
I'd prefer inheriting from base and add a implementation for locked?. This would make it easier to remove the Resilient (and Basic) lock implementations in the future, which I can't imagine we wouldn't do now that we have this awesome new lock implementation (and redis 2.x is very old at this point).
|
Thanks for this great change! Excited to get this pulled in. |
Fixes #553. In general, a better and more accepted approach to the master locking. Redis documentation even states:
https://redis.io/commands/set
Thanks @valo