Feat/multipool loadbalancer interface#400
Conversation
…balancing - Add LoadBalancer interface with Pick/Fallback methods - Add PoolMetrics interface exposing read-only pool stats - Extract load-balancing logic into lbs.go with built-in strategies: RoundRobinLB, LeastTasksLB, LeastWaitingLB - Add NewMultiPoolWithLB, NewMultiPoolWithFuncAndLB, NewMultiPoolWithFuncGenericAndLB for custom LB injection - Keep NewMultiPool/NewMultiPoolWithFunc/NewMultiPoolWithFuncGeneric unchanged for full backward compatibility - Add tests and benchmarks covering all three strategies and task types
…ces from custom LoadBalancer implementations
|
@panjf2000 Thanks so much for your time reviewing this! |
|
|
||
| if lbs != RoundRobin && lbs != LeastTasks { | ||
| // NewMultiPoolWithLB instantiates a MultiPool with a given LoadBalancer. | ||
| func NewMultiPoolWithLB(size, sizePerPool int, lb LoadBalancer, options ...Option) (*MultiPool, error) { |
There was a problem hiding this comment.
Instead of creating this function for each pool, we should consider using functional options for this, check out options.go
There was a problem hiding this comment.
@panjf2000 I have a question about the MultiPool option design.
Current constructor:
func NewMultiPool(size, sizePerPool int, lbs LoadBalancingStrategy, options ...Option) (*MultiPool, error)
The existing options ...Option is for sub-pools.
LoadBalancingStrategy belongs to MultiPool itself, not sub-pools.
Since Go doesn't allow two variadic parameters,
does it mean I need to change the sub-pool options from a variadic to a slice?
Like:
func NewMultiPool(size, sizePerPool int, poolOpts []Option, multiOpts ...MultiPoolOption)
Could you please confirm this?
Thank you!
There was a problem hiding this comment.
Yes, this is the tricky one. I don't have a feasible solution for that.
panjf2000
left a comment
There was a problem hiding this comment.
Besides, please fix the lint issues!
| return leastTasksPick(pools) | ||
| } | ||
|
|
||
| func (l *leastTasksLB) Fallback(pools []PoolMetrics) int { |
| return idx | ||
| } | ||
|
|
||
| func (l *leastWaiting) Fallback(pools []PoolMetrics) int { |
|
|
||
| if lbs != RoundRobin && lbs != LeastTasks { | ||
| // NewMultiPoolWithFuncAndLB instantiates a MultiPoolWithFunc with a given LoadBalancer. | ||
| func NewMultiPoolWithFuncAndLB(size, sizePerPool int, fn func(any), lb LoadBalancer, options ...Option) (*MultiPoolWithFunc, error) { |
|
|
||
| if lbs != RoundRobin && lbs != LeastTasks { | ||
| // NewMultiPoolWithFuncGenericAndLB instantiates a MultiPoolWithFuncGeneric with a given LoadBalancer. | ||
| func NewMultiPoolWithFuncGenericAndLB[T any](size, sizePerPool int, fn func(T), lb LoadBalancer, options ...Option) (*MultiPoolWithFuncGeneric[T], error) { |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #400 +/- ##
==========================================
- Coverage 95.11% 94.82% -0.29%
==========================================
Files 14 15 +1
Lines 798 831 +33
==========================================
+ Hits 759 788 +29
+ Misses 26 24 -2
- Partials 13 19 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Come to think of it, the LoadBalancer interface seems like overkill. As fancy as it may sound, I doubt many users will implement their own LoadBalancer interfaces. Correct me if I'm wrong, but it seems that all we want is a new load-balancing strategy - |
|
@panjf2000 I agree that making it a standalone interface is overkill. Meanwhile, this also exposes an issue: the current options are only for sub-pools. |
|
So it seems this PR might need to be put on hold for now. |
|
Would you like me to create a new branch and submit a minimal PR that only adds the LeastWaiting strategy as an enum? |
I expect We can revisit this feature when I hear more voices requesting it. "Premature optimization is the root of all evil." |
just revamp this PR and simply implement the new load-balancing strategy. we can open a new PR for the load-balancing interface in future, provided we really need it. |
Got it, I’ll revise this PR to implement the LeastWaiting load-balancing strategy directly. We’ll leave the LB interface refactor for a separate PR later if needed. Thanks for your advice! |
Description of new feature
PR: Introduce
LoadBalancerinterface for extensible MultiPool load balancing1. Are you opening this pull request for bug-fixes, optimizations or new feature?
New feature — with an internal refactor that is fully backward compatible.
2. Please describe how these code changes achieve your intention.
Problem
The current load-balancing strategy in
MultiPool,MultiPoolWithFunc, andMultiPoolWithFuncGenericis implemented as a closed enum (LoadBalancingStrategy) with aswitchinside anext()method. This has three limitations:next()is copy-pasted verbatim across all three MultiPool variants.RoundRobincounter (mp.index) lives on the pool struct rather than the strategy itself.Solution
Introduce two interfaces in a new
lbs.gofile:All three pool types (
*Pool,*PoolWithFunc,*PoolWithFuncGeneric[T]) already satisfyPoolMetricsvia the embedded*poolCommon— no extra code needed.Built-in strategies become small structs:
NewRoundRobinLB()NewLeastTasksLB()NewLeastWaitingLB()Three new constructors are added — one per MultiPool variant — that accept any
LoadBalancer:The existing
NewMultiPool/NewMultiPoolWithFunc/NewMultiPoolWithFuncGenericsignatures are unchanged — they now delegate to the new constructors internally after converting the enum to the corresponding built-inLoadBalancer. Zero breaking changes.Custom strategy example
Why
LeastWaitingLBis worth adding as a built-inBenchmarks below show that under mixed workloads (uneven task durations — the most common real-world scenario for IO-bound services),
LeastTasksLBdegrades severely becauseRunning()is a poor proxy for actual backlog pressure.LeastWaitingLBusesWaiting()instead and avoids the problem:LeastWaitingLBis also shown as a concrete example of what a customLoadBalancerimplementation looks like, making it a useful reference for users building their own strategies.3. Please link to the relevant issues (if any).
No existing issue. This PR is self-contained and was designed to be fully backward compatible.
4. Which documentation changes (if any) need to be made/updated because of this PR?
The README section covering
MultiPoolshould be updated to mention:NewMultiPoolWithLB/NewMultiPoolWithFuncAndLB/NewMultiPoolWithFuncGenericAndLBconstructorsLoadBalancerandPoolMetricsinterfacesNewLeastWaitingLB()strategyI am happy to update the README as part of this PR if the overall direction is accepted.
4. Checklist
Scenarios for new feature
Breaking changes or not?
No
Code snippets (optional)
Alternatives for new feature
None.
Additional context (optional)
None.