fix(least_connections) change setState order #128
Conversation
Add or remove the address to the available addresses binary heap in the correct order, i.e. setting its available values before making it actually available and, in the same sense, making it unavailable before setting its unavailable values.
|
@locao there is no test showing faulty behavour. Can you explain what this fixes exactly? |
|
This is pretty much the same as #126. The difference here is that when an address is set as unhealthy This tries to avoid a potential race condition, I wasn't able to reproduce the issue in an automated test. |
ghost
requested a review
from 
| @@ -137,7 +137,7 @@ function lc:getPeer(cacheOnly, handle, hashValue) | |||
| self.binaryHeap:pop() | |||
| end | |||
| address = self.binaryHeap:peek() | |||
| until address == nil or not (handle.failedAddresses or EMPTY)[address] | |||
| until address == nil or (not (handle.failedAddresses or EMPTY)[address] and address.host) | |||
There was a problem hiding this comment.
This feels like adding complexity when we shouldn't expect address.host to be nil at all. Although I agree, it might stop the crash! My concern though is that this line is already hard to enough to read.
Perhaps instead of this, line 163 should read in a way that is closer to how the other balancers work - they all check for disabled and available before calling getPeer(). So maybe this would do the same job (i.e. prevent the crash even if we don't understand how it happens), and if nothing more would make the balancers more similar?
if address == nil or not address.available or address.disabled then
...
|
Closing this due to lack of activity. Please re-open if needed. |
Add or remove the address to the available addresses binary heap in the correct order, i.e. setting its available values before making it actually available and, in the same sense, making it unavailable before setting its unavailable values.