-
Notifications
You must be signed in to change notification settings - Fork 911
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve task scheduler rate limiter #3860
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a decorator pattern here instead of a flag argument? That would follow the open-closed principle and reduce branching/entanglement. For example:
fx.Decorate(func(rateLimiter, cfg) RateLimiter {
return &rateLimiterShadow{rateLimiter, cfg.EnableRateLimiterShadowMode}
}
type rateLimiterShadow {
RateLimiter
UseShadow dynamicconfig.BoolPropertyFn
}
func (t *rateLimiterShadow) Wait(...) ... {
if t.UseShadow {
} else {
}
}
Yeah that's a good idea. Not that easy though since different rate limiter requests need different metrics tag (and we need a way to pass those information in) But for this change, consider it as a debug build, and all shadow related code will be deleted in next release after the issue was fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be better if this PR were split up into individual changes and we didn't use flag arguments, but given that this will be removed next release, LGTM.
000dca9
to
d3bb8f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's difficult for me to review the changes in common/tasks/interleaved_weighted_round_robin.go because the loop involved is so immense. If there's enough time, I'd encourage you to do some preparatory refactoring before adding this change. In particular, there's a complicated mix of blocking, non-blocking checks, usage of loop labels, and synchronization in one function. I'd encourage a more idiomatic Go approach where we use goroutines and select channel loops as opposed to nested loops with non-blocking ready checks
What changed?
Why?
How did you test it?
Potential risks
Is hotfix candidate?