-
Notifications
You must be signed in to change notification settings - Fork 165
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
FEATURE rueidislimiter: Introduce RateLimitOption to RateLimiterClient #681
Conversation
rueidislimiter/limit.go
Outdated
type RateLimitOption func(*RateLimit) | ||
|
||
func WithLimit(limit int) RateLimitOption { | ||
return func(rl *RateLimit) { | ||
rl.limit = limit | ||
} | ||
} | ||
|
||
func WithWindow(window time.Duration) RateLimitOption { | ||
return func(rl *RateLimit) { | ||
rl.window = window | ||
} | ||
} |
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.
type RateLimitOption func(*RateLimit) | |
func WithLimit(limit int) RateLimitOption { | |
return func(rl *RateLimit) { | |
rl.limit = limit | |
} | |
} | |
func WithWindow(window time.Duration) RateLimitOption { | |
return func(rl *RateLimit) { | |
rl.window = window | |
} | |
} | |
type RateLimitOption func(RateLimit) RateLimit | |
func WithLimit(limit int) RateLimitOption { | |
return func(rl RateLimit) RateLimit { | |
rl.limit = limit | |
return rl | |
} | |
} | |
func WithWindow(window time.Duration) RateLimitOption { | |
return func(rl RateLimit) RateLimit { | |
rl.window = window | |
return rl | |
} | |
} |
Hi @altanozlu, we can have better performance and fewer allocations with the above change.
Also, is there a case we want to adjust only one of the limit
or window
? If no, we should probably merge these two WithLimit
and WithWindow
.
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.
Hey, went into another route and changed RateLimitOption as a struct, since nothing is exposed to user, changing it into something else (e.g. type RateLimitOption func(RateLimit) RateLimit
) shouldn't be a problem in the future.
Now there is only 1 extra allocation when RateLimitOption is used.
It might be a little unorthodox so let me know if I should stick to conventional option usage.
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.
This one looks good to me. Thanks @altanozlu!
RateLimitOption is currently a struct but in the future it can be converted to another type.
Currently it's not possible to use instance of RateLimiter and share it with different use cases.