Repository navigation
grpc/load_balancing: Remove unused OneSubchannelPicker. - #2928
Conversation
| impl Picker for WakeUpPicker { | ||
| fn pick(&self, _request: &RequestHeaders) -> PickResult { | ||
| if !self.triggered_work.swap(true, Ordering::Relaxed) { | ||
| self.work_scheduler.schedule_work(None); |
There was a problem hiding this comment.
The None here is semantically significant. We can't just assume that. That should be passed in when creating. And we should give this a better name that works in this new context, like OneShotWorkPicker. But honestly, having two separate implementations of this seems fine. There's no need to manually compress the source code.
There was a problem hiding this comment.
I'm not sure I understand about the None being semantically significant? It is significant to schedule_work but both pickers are passing it, so I'm not entirely clear what the concern is on that point?
As for the other point, that's valid - I'm happy to rename (but don't particularly have a strong preference as to the name) or drop the consolidation entirely (and rewording the PR to just be about removing redundant OneSubchannelPicker). Preference?
There was a problem hiding this comment.
The meaning of the work object is determined by the LB policy. None happens to be what they used, but they might have needed something different and means nothing by itself. We shouldn't be manually compressing the code: seeing places that look the same and combining them. We can however combine things that are semantically the same, and to do that here we have to let the caller provide the None.
There was a problem hiding this comment.
Ah! I see what you mean now. And I agree that means we shouldn't be munging those things together, even if it would behave properly (for now). As a result, I've pulled that change out and just made this about removing the OneSubchannelPicker duplication. I will probably add a variant to pick_first to be explicit about it's idle case, but I can handle that in the PR around it. Thanks!
| pub(crate) struct OneSubchannelPicker { | ||
| sc: Arc<dyn Subchannel>, | ||
| } | ||
| pub(crate) struct QueuingPicker; |
There was a problem hiding this comment.
In Java we have a FixedResultPicker. You provide it the PickResult when constructing and it always returns that thing. You can then use it for queuing, single-subchannel, failing. It has been convenient.
There was a problem hiding this comment.
Yeah - as noted this is not actually new, the diff just mangled this horribly for some reason. The QueueingPicker has been in there for a while. A refactor of that might need to be a separate discussion/PR.
There was a problem hiding this comment.
I think this goes back to the "don't manually compress the code." As something is used once or two places moving it every time that changes. Instead, we have utilities that are likely to be used often. And a FixedResultPicker would be able to take the place of both OneSubchannelPicker and QueuingPicker.
You can add FixedResultPicker now, and use it in place of OneSubchannelPicker. Then in the future we could replace QueuingPicker with FixedResultPicker.
There was a problem hiding this comment.
I think I'll leave that for a future PR - this was largely to remove some review burden elsewhere, and I don't want to complicate matters around that for the moment at the risk of slowing things down. The main issue with OneSubchannelPicker is that the one in mod.rs is not being utilized, so probably shouldn't be there.
nathanielford
changed the title
# Conflicts: # grpc/src/client/load_balancing/mod.rs
nathanielford
changed the title
2 participants
Motivation
OneSubchannelPickercurrently exists in bothmod.rsand inpick_first.rs. Since it's only used bypick_first, it should only exist private to that LB.Solution
Remove the unused
mod.rscopy ofOneSubchannelPicker.