Little refactoring of locking logic #89
Conversation
There was a problem hiding this comment.
Thank you for your contributions, btw! 💚
I'll leave this as a comment for now, let's see what @leandromoreira says about the new style :)
| @@ -42,7 +42,7 @@ def initialize(servers = DEFAULT_REDIS_URLS, options = {}) | |||
| RedisInstance.new(server) | |||
| end | |||
| end | |||
| @quorum = (servers.length / 2).to_i + 1 | |||
| @quorum = servers.length / 2 + 1 | |||
lib/redlock/client.rb
Outdated
| time_elapsed = timed do | ||
| @servers.each do |server| | ||
| if server.lock(resource, value, ttl, allow_new_lock) | ||
| locked += 1 |
There was a problem hiding this comment.
TBH, I don't think this easier to read, especially mutating locked in an conditional.
If it's only about the timed function having 2 return values, you could still do
locked = 0
time_elapsed = timed do
locked = @servers.select { |s| s.lock resource, value, ttl, allow_new_lock }.size
endbut on the other hand, maybe it's only me, and I wouldn't say I had a strong opinion on this one :) So if you insist, please go ahead.
There was a problem hiding this comment.
Ya, one of the main reasons was 2 return values. Other, not critical in this case, is avoiding extra array allocation.
While original code is still very simple to understand it fast, personally I find it more clear when .select-like methods are using obvious explicit boolean-like methods inside it, like .locked? for example, to filter collection. And for doing something on collection, I would prefer just iterating using .each.
There was a problem hiding this comment.
hmm, so only .each may have side-effects? That's quite confining, isn't it?
How about
@servers.map { |s| s.lock resource, value, ttl, allow_new_lock }
.select(&:itself) # Or .select { |locked| locked }
.sizewrt. to the extra array allocation: Seems a bit too detailed for me, without having any benchmarks/measurements.
But again, I don't have any strong opinions on any of this. Maybe @leandromoreira, but otherwise we can also just merge this in, from my pov.
There was a problem hiding this comment.
I'm with @maltoe I don't see any gains with this (without bench) refactoring and I think the old code is simpler.
There was a problem hiding this comment.
FWIW There's is a .count method on arrays:
@servers.count{ |s| s.lock resource, value, ttl, allow_new_lock }There was a problem hiding this comment.
@maltoe Agreed with your initial comment, so updated taking it into account.
There was a problem hiding this comment.
@kitsunde Yes, but [1, 2, false].count == 3
There was a problem hiding this comment.
When block is provided, count returns elements for which block evaluates to truthy value. So this suggestion is good here.
There was a problem hiding this comment.
Oh nice, today I learned 👍
fatkodima
force-pushed
the
refactor-locking
branch
from
bd64781
to
ca19abe
Compare
lib/redlock/client.rb
Outdated
| time_elapsed = timed do | ||
| @servers.each do |server| | ||
| if server.lock(resource, value, ttl, allow_new_lock) | ||
| locked += 1 |
There was a problem hiding this comment.
I'm with @maltoe I don't see any gains with this (without bench) refactoring and I think the old code is simpler.
.timedmethod now is only concerned about just returning measured time.And while not a real problem in this case, but refactored code avoids extra array allocation. Also it improves understandability and is more straightforward, as for me.