grpc/lb: add SubchannelUpdate and remove LbPolicy::subchannel_update - #2873
Conversation
This change leverages the existing WorkScheduler to deliver subchannel updates instead of requiring LB policies to know which child created which subchannel.
Also stop producing Idle state updates since Idle is already reported by the return value of new_subchannel.
Note that pick first is the only LB policy that actually cares about subchannel state, since it is the only leaf policy. work() simply downcasts data to a SubchannelUpdate and feeds it to the existing subchannel_update logic, kept as-is as a normal method instead of implemented on the trait.
Remove the lock that caused potential races and deadlocks and instead route everything to happen synchronously through work(). The policy now creates its own work scheduler to use with new_subchannel which forwards updates from the "internal" subchannel to the shared "external" subchannels' work schedulers (in work()). Note that the work scheduler may be called before new_subchannel even returns, so the work scheduler needs to be resilient to that.
graceful switch, lazy, and round robin just delete subchannel update (and testing updates). child manager gets much simpler without having to track subchannels (and testing updates). We also can make the wrapped ChannelController implementation hold direct references into the child state to avoid the post-processing step.
schedule_subchannel_update is added so a test can simulate a state change the way the channel would: by scheduling it on the creating policy's scheduler rather than calling into the policy directly. StubPolicy's subchannel_update is removed; stubs that observe subchannel updates now implement work() instead.
| /// new state. Policies should generally pass the WorkScheduler they were | ||
| /// given in [`LbPolicyOptions`] so the update is routed back to them. |
There was a problem hiding this comment.
Why does the load balancer need to provide a WorkScheduler back to the channel? Since the channel already supplied one when creating the LB, can’t the channel assume it's the same one?
There was a problem hiding this comment.
This allows direct forwarding to a child policy that creates a subchannel. Otherwise how does a parent know how to route the subchannel update? It needs to intercept new_subchannel and then...remember which subchannel was created by which child (and so there's no benefit here).
This way the routing information can be built into the work scheduler itself.
There was a problem hiding this comment.
Ah, I see. The parent LB needs a way to intercept the schedule_work call from the subchannel.
| // call. | ||
| /// Schedules a call into the LbPolicy's work method. Multiple work calls | ||
| /// carrying a `data` payload of `None` may be coalesced with one another. | ||
| fn schedule_work(&self, data: Option<WorkData>); |
There was a problem hiding this comment.
What do you think about turning data into an enum to provide stronger type safety for common cases?
#[non_exhaustive]
enum WorkItem {
SubChannelUpdate(SubchannelState)
Work(WorkData)
None
}There was a problem hiding this comment.
I don't think this provides any stronger type safety, it just converts a downcast into a match.
I did consider this, but I think it's unnecessary and just adds another type into this system.
EDIT: Also, it requires implementers of work that don't ever call new_subchannel (which is everyone besides pick_first btw) to include SubchannelUpdate in their match arms.)
In the future I'll also be adding a subchannel health state struct, and arbitrary payloads for producers.
One thing I do still want to try out is limiting the data that can be sent to the work scheduler provided to new_subchannel so that it can only be used for a SubchannelUpdate. But that's a smaller refinement, I think, and will have more bearing when we get to the producer stuff.
2 participants
This change leverages the existing WorkScheduler to deliver subchannel updates instead of requiring LB policies to know which child created which subchannel.
This PR is split across 6 commits that touch different pieces in isolation: the LB module definition, channel impl, the different LB policies, and finally the existing test fakes to keep them working.
I have a follow-on PR that shares a ton of duplicated testing code which I noticed is common between all the LB policies. I'll send that after this is done, but don't look too too hard at
test_utils.rsor the tests and suggest nits/minor improvements (until that PR).