Stylistic improvements #6
Conversation
This function was called only once and it is a one-liner.
Writing `sentinels[i] = v` is shorter than `sentinels = append(sentinels, v)`. Also, why we are here, change `value, err` returns to idiomatic `nil, err`.
| return pool | ||
| } | ||
| s.mu.Unlock() | ||
| newPool := s.newPool(addr) | ||
| s.mu.Lock() | ||
| p, ok := s.pools[addr] | ||
| if ok { |
There was a problem hiding this comment.
This p is now called pool like the identical check earlier in the function.
| @@ -200,10 +191,8 @@ func (s *Sentinel) newPool(addr string) *redis.Pool { | |||
| // close connection pool to Sentinel. | |||
| // Lock must be hold by caller. | |||
| func (s *Sentinel) close() { | |||
| if s.pools != nil { | |||
There was a problem hiding this comment.
We don't need this nil check because len(nil) == 0; the for loop will do nothing in that case.
| if ok { | ||
| s.mu.Unlock() | ||
| defer s.mu.Unlock() | ||
| if pool, ok := s.pools[addr]; ok { | ||
| return pool | ||
| } | ||
| s.mu.Unlock() |
There was a problem hiding this comment.
Since there is a defer we don't need this Unlock anymore?
I am not sure, but I think the initial idea of lock usage in this func could be avoid locking while calling s.newPool(addr) (which can contain user supplied function to get Pool). This pr changes behaviour a bit.
There was a problem hiding this comment.
The original behaviour is still there, isn't it?
s.mu.Unlock()
newPool := s.newPool(addr)
s.mu.Lock()
There was a problem hiding this comment.
Oh, I see, was confused by defer and thought that the entire function meant to be under lock now, never seen this pattern before - interesting approach
|
Thanks, looks awesome! Left one note where I think it's better to keep previous style if there is no other reason for a change. |
|
Thanks! |
deferto unlock mutexredis.Stringsandredis.Stringhelpers more