Skip to content

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
28 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