Skip to content
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

4.x: Concurrency limits module, and support in Helidon WebServer #9295

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

tomas-langer
Copy link
Member

@tomas-langer tomas-langer commented Sep 27, 2024

Description

Resolves #8897
Resolves #9229

Documentation

This PR introduces

  • a new module common/concurrency/limits that provides API and SPI for concurrency limit implementations, and a couple of default implementations (AIMD, Fixed).
  • a new module webserver/concurrency-limits that provides feature with a filter to impose limits within a filter in routing
  • update of webserver/webserver to use a Limit instead of a Semaphore in connection handlers (backward compatible)

Configuration reference will be in the generated documentation.

Configure limits on WebServer

This will configure limit for a server listener (configurable per listener), enforced on the connection level (i.e. outside of routing and filters).

The default behavior is the same (unlimited).
If server.max-concurrent-requests is configured (value is not -1), it will be used and concurrency-limit configuration for the listener will be ignored.

Configuration:

server:
  port: 8080
  concurrency-limit:
    aimd: # `limit type`
      # AIMD limit configuration

Configure limits for routing

This will configure limit for as a server feature, enforced in an HTTP filter.
Configuration:

server:
  features:
    limits: # the feature is called `limits`
      enabled: true
      concurrency-limit: # `limit` configuration of the `limits` server feature
        fixed: # `limit type`
          permits: 1
          queue-length: 10

@tomas-langer tomas-langer self-assigned this Sep 27, 2024
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Sep 27, 2024
Copy link

@arouel arouel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great rework and integration

@vasanth-bhat
Copy link

vasanth-bhat commented Sep 30, 2024

Have few clarifications

  1. Going through Readme , there is mention about 'Semaphore' concurrency limit type in addition to aimd & BulkHead. Is this same as. "basic" (BasicLimit)? Also how is this different from specifying. "server.max-concurrent-requests"?

  2. For the BulkHead, it would be good to have metrics support , around queueing , for example % queue full. This is important in production to monitor the queueing.

  3. For AMID , it would be good to have a metrics around the dynamic concurrency limit in use. A Histogram may be useful here.

@tomas-langer
Copy link
Member Author

After discussing the needs in Helidon, we must do this using the permit approach, so this can be reused in webclient.
I will refactor the PR.

@arouel
Copy link

arouel commented Sep 30, 2024

I wanted to bring up that metrics would be super important for production as @vasanth-bhat brought it up as well. Within my draft I figured quickly that the limiting works as expected but metrics would give me the insights I need to build convidence. Maybe this is something for a follow-up Pull Request, idk.

Signed-off-by: Tomas Langer <[email protected]>
@tomas-langer
Copy link
Member Author

Metrics must be a follow up, as it would require much more work. I cannot just add a dependency on helidon-metrics-api, as it would introduce it everywhere (maybe that is the solution, but it requires a bit more thought).
I will finish this PR as the introduction of concurrency limits. We can look into the metrics as a follow up (this would make sense for fault tolerance as well, not just concurrency limits).

@tomas-langer
Copy link
Member Author

Pushed a new version, updated original description to reflect new implementation.
I have implemented the fixed limit as a more general feature - it supports queing now, so we no longer need fault tolerance for this.
How to add this to metrics needs to be a follow up.

@tomas-langer
Copy link
Member Author

Created a follow up issue: #9304

@vasanth-bhat
Copy link

Went through the new changes.

I think it would help to clarify between the below 2 , with example to understand the difference between the two. Do both them provide same functionality but via different approaches ?

  1. Fixed Limit with queueing support at the connection level (or a server listener )
  2. Fixed limit with queueing as server feature enforced via filter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
3 participants