-
Notifications
You must be signed in to change notification settings - Fork 339
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
SCB-143 Add new rate limiter #235
Conversation
Codecov Report
@@ Coverage Diff @@
## master #235 +/- ##
==========================================
+ Coverage 68.72% 68.81% +0.08%
==========================================
Files 17 17
Lines 3562 3559 -3
==========================================
+ Hits 2448 2449 +1
+ Misses 938 936 -2
+ Partials 176 174 -2
Continue to review full report at Codecov.
|
pkg/httplimiter/httpratelimiter.go
Outdated
limiter := &HttpLimiter{RequestLimit: max, TTL: ttl} | ||
limiter.ContentType = "text/plain; charset=utf-8" | ||
limiter.HttpMessage = "You have reached maximum request limit." | ||
limiter.StatusCode = 429 |
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.
Use http.StatusTooManyRequests replace the magic number
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.
fixed
pkg/httplimiter/httpratelimiter.go
Outdated
w.Header().Add("X-Rate-Limit-Duration", limiter.TTL.String()) | ||
} | ||
|
||
func checkExsistence(sliceString []string, needle string) bool { |
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.
checkExsistence -> checkExistence
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.
fixed
if httpError != nil { | ||
w.Header().Add("Content-Type", this.tbLimiter.MessageContentType) | ||
w.Header().Add("Content-Type", this.httpLimiter.ContentType) | ||
w.WriteHeader(httpError.StatusCode) | ||
w.Write(util.StringToBytesWithNoCopy(httpError.Message)) | ||
util.Logger().Warn("Reached maximum request limit!", nil) |
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.
please log print the limit remote ip and request URI
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.
fixed
@little-cui Thanks for reviewing, I have fixed the code based on your comments, kindle review... |
Follow this checklist to help us incorporate your contribution quickly and easily:
[SCB-XXX] Fixes bug in ApproximateQuantiles
, where you replaceSCB-XXX
with the appropriate JIRA issue.mvn clean install
to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.