(refactor) Pick First LB state machine - #2784
Conversation
… ready during resolver update.
arjan-bal
left a comment
arjan-bal
left a comment
There was a problem hiding this comment.
Leaving initial comments, haven't reviewed the entire PR.
| PickFirstState::FirstPass(s) => !s.addresses.is_empty(), | ||
| PickFirstState::SteadyState(s) => !s.addresses.is_empty(), | ||
| PickFirstState::Ready(s) => !s.addresses.is_empty(), |
There was a problem hiding this comment.
I believe in all these cases, addresses must be present and the value can directly be true.
| FirstPassState::fresh_enter(ctx, addresses) | ||
| } | ||
|
|
||
| #[allow(clippy::unused_self)] |
There was a problem hiding this comment.
self is used in this method, so #[allow(clippy::unused_self)] can be avoided.
| } | ||
|
|
||
| fn work(self, ctx: &mut PickFirstContext<'_>) -> PickFirstState { | ||
| self.exit_idle(ctx) |
There was a problem hiding this comment.
There was a TODO about checking if it's safe to assume every call to work should trigger exit_idle. Has this been confirmed?
Also, all the TODOs seem to be removed, have they been addressed?
| #[cfg(test)] | ||
| impl PickFirstPolicy { |
There was a problem hiding this comment.
Can we move this to the test mod below?
| ) -> PickFirstState { | ||
| FirstPassState::fresh_enter(ctx, addresses) | ||
| } |
There was a problem hiding this comment.
Pickfirst should remain in idle state unless the idle picker is used or the channel calls exit_idle.
| Err(e) => { | ||
| self.state = PickFirstState::Idle(IdleState { | ||
| addresses: Vec::new(), | ||
| }); |
There was a problem hiding this comment.
Pick-first should probably enter one of the Transient Failure (TF) states here.
When a valid resolver update arrives while the balancer is in TF, it should start using the new address list immediately. However, if the balancer is in IDLE, it should wait until exit_idle() is called. We should add tests to verify this.
| ctx.controller.request_resolution(); | ||
| return PickFirstState::Idle(self); |
There was a problem hiding this comment.
The balancer must transition the channel out of IDLE here. The IDLE picker only triggers exit_idle() once. If the PF balancer remained in IDLE on receiving resolver updates, it will remain permanently stuck in the IDLE state.
2 participants
Motivation
The initial implementation of the PickFirst LB was a little tangled in how it implemented the Happy Eyeballs requirements. This meant it was handling some phases with special state and others without. Overall it made it hard to reason about.
Further, there was abehavior gaps between the original implementation and the spec: the original discards unselected addresses upon entering READY, causing subsequent reconnections from IDLE to only attempt the single failed address instead of sweeping all resolved backends.
Solution
This implementation is fairly different, using a state machine to hold state for each of the four states of Happy Eyeballs (Idle, FirstPass, SteadyState and Ready), and being clear about state transitions between them. The construction ensures the retention of appropriate information (i.e. addresses), and makes things easier to reason about.
As a bonus, this pass also eliminates a bunch of allocations that were unneeded. Gemini estimates about a 70% smaller memory footprint. As a double plus bonus, it now passes linting checks.