-
Notifications
You must be signed in to change notification settings - Fork 311
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
feat: gateway back pressure mechanism implementation #1847
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1847 +/- ##
=======================================
Coverage 25.54% 25.54%
=======================================
Files 116 116
Lines 28738 28736 -2
=======================================
Hits 7340 7340
+ Misses 21056 21054 -2
Partials 342 342
Continue to review full report at Codecov.
|
68f1ca2
to
a408bb7
Compare
removed the Ready for Review, until changes are addressed/pushed |
name: "max concurrent request: 10", | ||
maxConcurrentRequests: 10, | ||
totalReq: 10, | ||
}, |
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.
[minor] and subjective. I would prefer to see expected responses here, instead of computing them in:
require.Equal(t, tt.totalReq-tt.maxConcurrentRequests, resp503, "actual 503 resp different than expected.")
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 reason I have written it like this is, to make it more intuitive that anything over allowed concurrentRequests would be 503. tt.totalReq-tt.maxConcurrentRequests
mathematically indicates that. @lvrach WDYT?
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.
LGTM
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 fairly good, I've added very few comments, the blocking one is the one about doing atomic.LoadInt32(&concurrentRequests)
. Let me know if it doesn't make sense of course.
Up to you guys, I don't think it should be a blocker but definitely a nice
to have, especially if easy enough to setup.
…On Thu, 28 Apr 2022, 10:55 Aris Tzoumas, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In middleware/middleware.go
<#1847 (comment)>
:
> +
+func MaxConcurrentRequestsMiddleware(maxConcurrentRequests int) func(http.Handler) http.Handler {
+
+ var requests = make(chan struct{}, maxConcurrentRequests)
+ return func(next http.Handler) http.Handler {
+ //this is to make sure that we don't have more than `maxClient` in-memory at any point of time. As, having more http client than `maxClient`
+ // may lead to gateway OOM kill.
+ return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ if maxConcurrentRequests != 0 {
+ select {
+ case requests <- struct{}{}:
+ defer func() {
+ <-requests
+ }()
+ default:
+ http.Error(w, http.StatusText(http.StatusServiceUnavailable), http.StatusServiceUnavailable)
@fracasula <https://github.com/fracasula> we will selectively enable the
limit on specific customer(s), by default it is disabled. IMO the existing
metric, number of active connections should be enough for monitoring &
troubleshooting at this stage.
—
Reply to this email directly, view it on GitHub
<#1847 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJ2QQABWA3YEEXPAKROADLVHJHBFANCNFSM5TKYXPSQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
3fcdd9d
to
63b88b4
Compare
Description of the change
To avoid OOM kill of gateway in scenarios where we have a lot of sudden spike in gateway traffic or when DB is under pressure. We have introduced a back pressure mechanism that restrict on maximum number of active request in memory, by using counting Semaphore.
Notion Link
Type of change
Checklist:
Security