-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Load shedding #40142
Load shedding #40142
Conversation
This comment has been minimized.
This comment has been minimized.
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 great!
First, I like the overload algorithm you are using. Any reference on it?
Then, we have a connection limiter in Quarkus already, should we deprecate this in favor of the load shedding?
Also, I think it needs to be tested with long-running connection (gRPC streams, SSE, web sockets).
I'm also thinking that on overload, we may need to adjust the readiness Kube probe. WDYT?
The algorithm is a simplified version of https://github.com/Netflix/concurrency-limits/blob/master/concurrency-limits-core/src/main/java/com/netflix/concurrency/limits/limit/VegasLimit.java, I think there's a link in the javadoc already.
Ah do we? I had no idea. Where can I learn more?
Good point, I didn't try that at all. I'll check, but I doubt I'll see anything meaningful. The present implementation is heavily oriented to request/response style of interaction. Occasional stream likely won't do anything, and a streaming-heavy application would require a more involved implementation.
Hmm, I'm not sure about that. That would be super coarse-grained. |
7a4c509
to
c48e093
Compare
Just marked as ready for review. I fixed a couple of bugs in the implementation and added some rudimentary documentation. The feature is not very heavily tested. @franz1981 do you think it would be possible to test this in a perf lab? I don't really know how that works, but I guess we have a test that stresses Quarkus application above its capacity, where this would help? |
This comment has been minimized.
This comment has been minimized.
🙈 The PR is closed and the preview is expired. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Rebased and fixed the conflict. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Rebased and fixed a few tiny issues. I believe this is ready now. |
This comment has been minimized.
This comment has been minimized.
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 think we should merge it and iterate.
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 added two small comments. I don't mind you merging and addressing them later so that we avoid some more conflicts.
extensions/load-shedding/runtime/src/main/resources/META-INF/quarkus-extension.yaml
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
The overload detector uses a TCP Vegas based algorithm, as implemented by Netflix Concurrency Limiters. Priority load shedding uses 5 priority levels and 128 cohorts. A simple cubic function is used to determine the threshold that current CPU load has to reach to reject the current request.
Status for workflow
|
Status for workflow
|
Related to #36543