reader: make sure RDY is updated when connection count changes

[alpaker]

Fixes #182.

Previously, a connection's rdy attribute was decremented on message receipt; when it dipped low enough the connection would send a new RDY count of _connection_max_in_flight(). One consequence of this was that, when _connection_max_in_flight() changed (because either the connection count or the reader's max_in_flight attribute changed), each connection would eventually update to the new value.

The reader's total_rdy attribute was also decremented on message receipt. This guaranteed (under the assumption that some connection was receiving messages), that total_rdy would drop below max_in_flight, freeing up space for a new connection to send an initial RDY count of 1.

Now that these client-side attributes aren't updated with each message received, these two cases need to be catered for explicitly.

[alpaker]

@mreiferson RFR

[mreiferson]

Is it safe to always choose the 0th conn?

[alpaker]

I think you're right that it's not safe. We need to guarantee that the conn we select has RDY greater than the new connection_max_in_flight(). One can be shown to exist at this point, on the assumption that we're not in the abnormal regime where the connection count, counting the new conn, is greater than max_in_flight, but we're not guaranteed that an arbitrarily chosen conn satisfies that condition. (If we are in the abnormal regime, trying to set a non-zero RDY on the new conn is going to fail and we have to rely on periodic redistribution to give RDY to the new conn.)

Just resetting RDY on all the connections, as you suggest, would simplify the logic.

[mreiferson]

Thinking out loud — is it easier to just reset all conn RDY counts whenever we process a new connection? Rather than this odd separation between initial RDY and then proper per-conn max_in_flight after receipt of first message?

[alpaker]

I did try that out. But test expectations need to be changed if we do that, so instead I went with a change that involved less perturbation to current behavior. Happy to go either way.

[alpaker]

I think we don't want to unconditionally reset RDY counts on the other connections: some might be be testing the waters with RDY 1 and waiting for a message, so it would be wrong to update them to the connection max-in-flight. But it would work to update RDY on all connections that have RDY greater than the new per-connection max. With that additional condition, we also don't need any test changes. PR updated accordingly.

[mreiferson]

Sorry for the delay, I think this looks good.

Is there a relatively easy test we can add that fails on master and is fixed by this?

[alpaker]

It's straightforward to test, but requires multiple connections, so there's not a natural place to add the test. One option would be to refactor the integration test harness in test_reader.py to work with multiple connections. But a better direction might be to add a new test_rdy.py that can accumulate unit tests related to RDY count management, à la test_backoff.py.

[mreiferson]

But a better direction might be to add a new test_rdy.py that can accumulate unit tests related to RDY count management, à la test_backoff.py.

Let's try that! Thanks!

[alpaker]

@mreiferson Added the new test file. Sorry for the delay.

I moved some helper functions from test_backoff.rdy to a new reader_unit_test_helpers.py. Let me know if you don't like that refactoring.

[mreiferson]

awesome, thanks!

[smant]

What about 0.8.2 release with fixed #182 ?