Fix/service discovery #96
Conversation
Pull Request Test Coverage Report for Build 811
💛 - Coveralls |
cluster/etcd_service_discovery.go
Outdated
| if svMap, ok := sd.serverMapByType.Load(sv.Type); ok { | ||
| sm := svMap.(map[string]*Server) | ||
| delete(sm, sv.ID) | ||
| sd.mapByTypeLock.Lock() |
There was a problem hiding this comment.
Isn't it better to defer sd.mapByTypeLock.Unlock() right after locking?
There was a problem hiding this comment.
Given that there is only one flow deferring is slower and would be executed only after notifying the listeners
There was a problem hiding this comment.
You have a point, but if we panic in the next lines there's a chance we won't release the lock. Maybe wrap what really needs to be inside the lock in another method and call it from here?
There was a problem hiding this comment.
The scope of a defer is its function, it would get really ugly to create a function just to wrap the lock and deal with panics. A panic here would lead to the crash of the application, which seems the proper behavior.
There was a problem hiding this comment.
Ok, I created a wrapper for the lock, what do you think?
|
Great job 😍 |
Fixes #95