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

[Broker][Proxy][Function worker] Fix backpressure handling in Jetty web server configuration #14353

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Feb 17, 2022

Motivation

The current Jetty configuration doesn't properly handle backpressure. This is a problem when asynchronous request handling is used and there's high load. The requests will get handled and the asynchronous calls are made to the backend system and this might cause all requests to fail.

One of the main reasons to configure backpressure is to ensure that the system doesn't get overwhelmed and can handle successfully a certain amount of requests and reject the other requests. When more and more asynchronous request handling is happening, it is importance of having a backpressure handling that prevents too much work entering the system.

It should be possible to limit the number of concurrent requests that are handled at a time. There is a setting maxConcurrentHttpRequests, but that is not correctly implemented and has no impact.

This PR fixes the backpressure handling and improves the settings around backpressure control. The changes are made consistently for broker, proxy, function worker and websocket proxy, where Jetty is used in Pulsar.

Modifications

  • Fix maxConcurrentHttpRequests by using QoSFilter to limit concurrent requests

    • previous solution didn't limit concurrent http requests. The setting maxConcurrentHttpRequests was used to configure accept queue size. This didn't limit concurrent http requests.
  • Replace previous hardcoded connection limit of 10000 http connections with configurable setting maxHttpServerConnections

    • use Jetty's built-in connection limit instead of PulsarServerConnector's custom solution
  • Rate limiting should happen in the beginning of the filter chain

  • Let Jetty tune selectors and acceptors based on number of cores

    • JETTY_AVAILABLE_PROCESSORS=n environment variable can be used to override the number of cores reported by the OS
      • This is useful when CPU limit isn't set on k8s and the number of cores is the number of total cores available on the k8s node
      • use can also use -XX:ActiveProcessorCount=n to make Java's Runtime.getRuntime().availableProcessors() return n
  • Make accept queue capacity configurable by httpServerAcceptQueueSize setting

  • make the queue for Jetty's thread pool's bounded and configured with httpServerThreadPoolQueueSize setting

    • the default is unlimited and doesn't cause backpressure which is desired when the queue grows

@lhotari
Copy link
Member Author

lhotari commented Feb 17, 2022

btw. It would be useful to investigate the use of https://www.eclipse.org/jetty/javadoc/jetty-9/org/eclipse/jetty/servlets/DoSFilter.html for basic DoS protection.

@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@lhotari lhotari force-pushed the lh-improve-jetty-configuration-to-handle-backpressure branch 2 times, most recently from 5ff348d to c9e17a0 Compare April 8, 2022 12:28
@lhotari lhotari marked this pull request as ready for review April 8, 2022 13:28
@lhotari lhotari force-pushed the lh-improve-jetty-configuration-to-handle-backpressure branch 2 times, most recently from 9bf6c2a to 2cf16f7 Compare April 11, 2022 08:19
@lhotari lhotari changed the title [Broker] Improve Jetty configuration to handle backpressure properly [Broker][Proxy] Improve Jetty configuration to handle backpressure properly Apr 11, 2022
@lhotari lhotari changed the title [Broker][Proxy] Improve Jetty configuration to handle backpressure properly [Broker][Proxy] Fix backpressure handling in Admin API backend / Jetty configuration Apr 11, 2022
@lhotari lhotari requested a review from zymap April 11, 2022 12:09
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Really great work.
It is better to use builtin Jetty components

@eolivelli
Copy link
Contributor

@merlimat @rdhabalia @codelipenghui would you mind taking a look ?

Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

Great work @lhotari! I think we should also add this back pressure handling to the WorkerServer, which also runs a Jetty server.

- Fix maxConcurrentHttpRequests by using QoSFilter to limit concurrent requests
  - previous solution didn't limit concurrent http requests
- Replace previous hardcoded connection limit of 10000 http connections with configurable setting
  - use Jetty's built-in connection limit instead of PulsarServerConnector's custom solution
- Rate limiting should happen in the beginning of the filter chain
- Let Jetty tune selectors and acceptors based on number of cores
  - JETTY_AVAILABLE_PROCESSORS=n environment variable can be used to override the number of cores reported by the OS
    - This is useful when CPU limit isn't set on k8s and the number of cores is the number of total cores available on the k8s node
    - use can also use -XX:ActiveProcessorCount=n to make Java's Runtime.getRuntime().availableProcessors() return n
- Make accept queue capacity configurable
- Make thread pool queue capacity bounded and make it configurable
- Make hardcoded websocket thread pool size of 20 configurable
@lhotari lhotari force-pushed the lh-improve-jetty-configuration-to-handle-backpressure branch from 2cf16f7 to 04bfb4a Compare April 12, 2022 06:39
@lhotari
Copy link
Member Author

lhotari commented Apr 12, 2022

Great work @lhotari! I think we should also add this back pressure handling to the WorkerServer, which also runs a Jetty server.

@michaeljmarshall Thanks for pointing that out. I added a commit to cover Function Worker with similar changes.

@lhotari lhotari changed the title [Broker][Proxy] Fix backpressure handling in Admin API backend / Jetty configuration [Broker][Proxy][Function worker] Fix backpressure handling in Admin API backend / Jetty configuration Apr 12, 2022
@lhotari lhotari changed the title [Broker][Proxy][Function worker] Fix backpressure handling in Admin API backend / Jetty configuration [Broker][Proxy][Function worker] Fix backpressure handling in Jetty web server configuration Apr 12, 2022
@codelipenghui codelipenghui added this to the 2.11.0 milestone Apr 12, 2022
@lhotari lhotari merged commit ec38211 into apache:master Apr 12, 2022
lhotari added a commit to datastax/pulsar that referenced this pull request Apr 14, 2022
…eb server configuration (apache#14353)

* [Broker] Improve Jetty configuration to handle backpressure

- Fix maxConcurrentHttpRequests by using QoSFilter to limit concurrent requests
  - previous solution didn't limit concurrent http requests
- Replace previous hardcoded connection limit of 10000 http connections with configurable setting
  - use Jetty's built-in connection limit instead of PulsarServerConnector's custom solution
- Rate limiting should happen in the beginning of the filter chain
- Let Jetty tune selectors and acceptors based on number of cores
  - JETTY_AVAILABLE_PROCESSORS=n environment variable can be used to override the number of cores reported by the OS
    - This is useful when CPU limit isn't set on k8s and the number of cores is the number of total cores available on the k8s node
    - use can also use -XX:ActiveProcessorCount=n to make Java's Runtime.getRuntime().availableProcessors() return n
- Make accept queue capacity configurable
- Make thread pool queue capacity bounded and make it configurable

* [Functions] Add http backpressure handling for Functions worker's http server

(cherry picked from commit ec38211)
aparajita89 pushed a commit to aparajita89/pulsar that referenced this pull request Apr 18, 2022
…eb server configuration (apache#14353)

* [Broker] Improve Jetty configuration to handle backpressure

- Fix maxConcurrentHttpRequests by using QoSFilter to limit concurrent requests
  - previous solution didn't limit concurrent http requests
- Replace previous hardcoded connection limit of 10000 http connections with configurable setting
  - use Jetty's built-in connection limit instead of PulsarServerConnector's custom solution
- Rate limiting should happen in the beginning of the filter chain
- Let Jetty tune selectors and acceptors based on number of cores
  - JETTY_AVAILABLE_PROCESSORS=n environment variable can be used to override the number of cores reported by the OS
    - This is useful when CPU limit isn't set on k8s and the number of cores is the number of total cores available on the k8s node
    - use can also use -XX:ActiveProcessorCount=n to make Java's Runtime.getRuntime().availableProcessors() return n
- Make accept queue capacity configurable
- Make thread pool queue capacity bounded and make it configurable

* [Functions] Add http backpressure handling for Functions worker's http server
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
…eb server configuration (apache#14353)

* [Broker] Improve Jetty configuration to handle backpressure

- Fix maxConcurrentHttpRequests by using QoSFilter to limit concurrent requests
  - previous solution didn't limit concurrent http requests
- Replace previous hardcoded connection limit of 10000 http connections with configurable setting
  - use Jetty's built-in connection limit instead of PulsarServerConnector's custom solution
- Rate limiting should happen in the beginning of the filter chain
- Let Jetty tune selectors and acceptors based on number of cores
  - JETTY_AVAILABLE_PROCESSORS=n environment variable can be used to override the number of cores reported by the OS
    - This is useful when CPU limit isn't set on k8s and the number of cores is the number of total cores available on the k8s node
    - use can also use -XX:ActiveProcessorCount=n to make Java's Runtime.getRuntime().availableProcessors() return n
- Make accept queue capacity configurable
- Make thread pool queue capacity bounded and make it configurable

* [Functions] Add http backpressure handling for Functions worker's http server
@lhotari
Copy link
Member Author

lhotari commented Jun 2, 2022

There's a follow up PR #15637 to address some remaining gaps in the backpressure solution. Each context path had it's own rate limiter instance and that's why it's not currently behaving in an expected way. #15637 fixes this issue. Please review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker area/proxy doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants