cluster manager: change underlying call chain for drainConnections

[junr03]

Commit Message: cluster manager - change underlying call chain for drainConnections
Additional Description: previously drainConnections used ConnPool::startDrain which does not guarantee that non-idle connections will not be used for new streams. This change leverages the logic in onHostHealthFailure to close idle connections and mark non-idle connections as draining so that new streams are forced onto new connections.
Risk Level: low - API usage is opt in
Testing: existing tests.

Signed-off-by: Jose Nino jnino@lyft.com

[junr03]

@mattklein123 I wanted to get this up asap to get your take. I felt that all the logic in onHostHealthFailure was what we wanted out of ClusterManager::drainConnections() (especially because that function does not charge any stats specific to host health failure) but I am the first to admit that it is not the cleanest code. I can move things a bit around, but given the subsequent cleanup I wanted to do after @RyanTheOptimist changes in the conn pool it felt silly, so I opted for a todo. lmk what you think.

re: testing. The existing test obviously passes, but I am still working on a test that more explicitly makes sure that a new connection is establish for a stream even if the previous connection was not idle. I had the idea to look for how onHostHealthFailure is tested this morning so I will push an update after my morning meetings. Update: there's no easy way I can see to directly observe the behavior. The existing onHostHealthFailure test is basically the same as the existing integration test. I will merge this and continue to noodle on a better test.

[mattklein123]

Thanks for fixing. Small comment.

/wait

[mattklein123]

Cool, thanks!