-
Notifications
You must be signed in to change notification settings - Fork 59
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
fix #164 and respect ratelimit for bulk joins #165
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ence implementations, I'm sure this will be fine Clueless
} | ||
} | ||
|
||
func (r *RateLimiter) Throttle() { | ||
func (r *RateLimiter) Throttle(count int) { |
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.
Should there be a mutex on this internally to the ratelimiter, or is it/are you planning on handling it externally?
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.
The writer runs in with channel so there should be not concurrent access of this function
Co-authored-by: pajlada <rasmus.karlsson@pajlada.com>
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.
Looks good to me!
} | ||
if sb.Len() == len(baseMessage) { | ||
sb.WriteString(" #" + channel) | ||
channelsWritten++ |
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.
Would it be more clear to increment channelsWritten
after the if-else? I'm not sold on moving it or leaving it as is myself
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.
I was unsure as well, but this is probably good.
In a way it also documents each branch of the if statement
Ratelimiting implemented based on a slice with times in it. All the other examples of sliding window implementations looked super complicated to me and not so straight forward.
I'm not sure if this is the correct way to solve this problem, but at least my tests run now. Which they haven't done for any implementation before as you can see by the build history.
One slight weird thing still is I added a 100ms extra sleep, because on CI it kept failing, locally I could run it fine. This is slightly alarming because I'm wondering if on different systems it could behave even differently.
But running
go test -race
brings up no issues.