Bug in calculating MaxCull

[epgsql]

Bug in calculating MaxCull

[seth]

Hey Dan,

Thanks so much for reporting this. I'm reading it over and am pretty sure you've found a bug. Yes, MaxCull is intended to ensure that the pool size doesn't drop below initCount when removing stale members.

I will get this fixed.

[dcheckoway]

Thanks @seth! PR forthcoming...

[seth]

Hi again,

I think the culling is doing the right thing in the end. Perhaps it could be improved to read more easily. Here's why MaxCull = FreeCount - InitCount isn't the right thing:

Consider a pool with initCount = 2 and maxCount = 10. Now suppose that the pool is full with 10 members, 2 of which are in use, and the rest "stale". We'd then set MaxCull = 8 - 2 = 6 which is too low since we should cull 8 members to reduce the pool size to 2.

Note that the code that finds expired members only returned free members that match the staleness criteria -- so we'll never remove in-use nor recent members. The use of lists:sublists means that if MaxCull is larger than the expired free count, we'll just cull the expired free members.

I'm going to a add a test case to cover this. Right now, the tests pass with either MaxCull approach which isn't great.

[dcheckoway]

I think there's still a bug in the calculation. Your example works because InitCount == InUseCount. Consider the case where InUseCount > InitCount:

maxCount = 10
initCount = 2
InUseCount = 4
FreeCount = 6

The code on master:

MaxCull = FreeCount - (InitCount - InUseCount)

would equate to:

MaxCount = 6 - (2 - 4) = 8

It should never cull deeper than FreeCount, right? I think you might want to do something along the lines of:

MaxCount = FreeCount - max(InitCount - InUseCount, 0)

Check me on that...

[seth]

Have you found a case where you are seeing the over-cull behavior? You are right that MaxCull at present will sometimes be too large, but because the candidates for culling are only those that are stale and free, I don't think we'll end up over-culling.

I won't have time at the computer till later, but I think the next step is to add some further test cases :)
And then decide how to make the code more clear (haven't worked through your suggestion). And I want to think about whether it matters to compute an "exact" max cull or not :)

Thanks again!

[dcheckoway]

Kind of a long story, but we discovered this accidentally as a result of a bug in our code where we were inadvertently returning members to the pool when they were still actually in use. The culling closed what looked like stale members (rightly so) and pulled the rug right out from under us.

That's our issue to deal with, but it got me looking at the cull methodology, and...here we are. :-)

[seth]

I've added some additional test coverage around member culling that I believe covers both the example I gave and your recent example.

In the process I found a bug that returning the same pid twice broke the pool which is now fixed.

I'll still think over a different MaxCull code -- either comments, or different calc, but had limited time so focused on adding tests first.

I'm inclined to close this because I think with the new tests we are seeing that, aside from being confusing, the culling operation is working as expected. But I'll wait to see if you have either evidence or comments otherwise.

Thanks.

[seth]

assuming no news is good news.