Fix heap update operation for least-loaded picker #61
Conversation
| @@ -86,23 +86,20 @@ type leastLoadedConnHeap []*leastLoadedConnItem | |||
| type leastLoadedConnItem struct { | |||
| conn conn.Conn | |||
| load uint64 | |||
| tiebreak uint64 | |||
| tieBreak uint64 | |||
There was a problem hiding this comment.
We have nextTieBreak with a capital B elsewhere, so I capitalized it here for consistency that this represents two words.
| entry := (*h)[0] | ||
| entry.load++ | ||
| entry.tieBreak = nextTieBreak |
There was a problem hiding this comment.
I think we should be doing this here, instead of outside of this function (as was done before), so that the Fix call below takes this value into consideration.
There was a problem hiding this comment.
Ouch, nasty bug. Very nice looking test though, that definitely seems rather convincing. To my eyes the explanation makes sense and there is nothing obviously wrong with the code, so LGTM. That said, it does make me wish we could get a bit more battle-testing on this (Aside from production adoption, I'd really like to see if we could sneak in some load tests in CI.) I guess that's why we're still 0.x for now.
@jchadwick-buf, @lrewega was trying out the least-loaded picker and it panick'ed! 😱
It turns out, the
updateoperation -- which reconciles the heap with a new set of connections provided from a resolve -- was iterating from start to finish of the slice, but it would also change the length of the slice while iterating, when it decided it needed to pop an item that should no longer be present.So the panic was an out-of-bounds slice index. But the iteration is incorrect for more than just that reason: removing an item from the heap that way will also re-order items, to sift things up and down to preserve heap invariants within the slice. So to proceed iterating through the slice means we might visit the same item more than once and fail to visit some items, as their order may have changed underneath us.
So now the logic does a single pass through the slice to remove unneeded items, by simply overwriting them (and setting to nil if necessary). So at the end of the first pass, all that is left in the slice are the items in the new set of connections, compacted to the beginning of the slice (everything after is set to nil). Then we append any new connections. And at the very end, we re-heapify to restore heap invariants after all of that.
This adds a test that is hopefully pretty convincing that it all works correctly now. The test is a sequence of operations, including acquiring connections, releasing them, and updating the resolved set.