Skip to content
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

Merged
merged 1 commit into from
Nov 28, 2024

Conversation

altanozlu
Copy link
Contributor

Currently it's not possible to use instance of RateLimiter and share it with different use cases.

Comment on lines 9 to 15
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
}
}
Copy link
Collaborator

@rueian rueian Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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.
@rueian rueian merged commit c256ef2 into redis:main Nov 28, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants