Add QOS filtering of overlord requests by cryptoe · Pull Request #18033 · apache/druid

cryptoe · 2025-05-26T07:10:29Z

Adds QOS filtering for the overlord so that health check threads are not blocked.

kfaraz

Thanks for the improvement, @cryptoe .
I have left some suggestions.

kfaraz · 2025-05-26T07:32:45Z

 public class ServerConfig
 {
  public static final int DEFAULT_GZIP_INFLATE_BUFFER_SIZE = 4096;
+  public static final int DEFAULT_NUM_PACKING_THREADS = 30;


What does "packing" signify here?

its the extra threads we add . Couldn't really find a good word.

kfaraz · 2025-05-26T07:35:07Z


-    threadPool.setDaemon(true);
-    jettyServerThreadPool = threadPool;
+    jettyServerThreadPool.setDaemon(true);


Original code was more appropriate where jettyServerThreadPool was assigned only after the value threadPool was fully baked.

Otherwise, the doMonitor method might emit erratic metrics.

The ideal fix here would to be to make jettyServerThreadPool non-static but that is not needed in this PR and it would require other clean up too.

Fixed the jettyServerPool to get initialized in one go with the relevant threads.

The daemon stuff should not effect the monitor emitting metrics.
Yeah there is a race here. Agreed we can fix it as part of another PR.

Are the changes to this file really needed in this PR?
I am not sure how it affects the main QoS change.

kfaraz · 2025-05-26T07:45:23Z

+      final QueuedThreadPool queuedThreadPool = (QueuedThreadPool) server.getThreadPool();
+      final int maxThreads = queuedThreadPool.getMaxThreads();


Might be nice to have the logic to compute the max threads commoned out somewhere that can be used here. It seems weird to need a handle to the actual ThreadPool object just to determine the max threads.

kfaraz · 2025-05-26T07:45:57Z

    }
  }
+
+  protected static boolean addQOSFiltering(ServletContextHandler root, int threadsForOvelordWork)


Please add a short javadoc.

Suggested change

protected static boolean addQOSFiltering(ServletContextHandler root, int threadsForOvelordWork)

protected static boolean addQosFiltering(ServletContextHandler root, int threadsForOvelordWork)

kfaraz · 2025-05-26T07:51:21Z

+   * As QOS filtering is enabled on overlord requests, we need to update the QOS filter paths in
+   * {@link org.apache.druid.cli.CliOverlord#addQOSFiltering(ServletContextHandler, int)} when a new jersey resource is added.


Suggested change

* As QOS filtering is enabled on overlord requests, we need to update the QOS filter paths in

* {@link org.apache.druid.cli.CliOverlord#addQOSFiltering(ServletContextHandler, int)} when a new jersey resource is added.

* Since QoS filtering is enabled for Overlord requests, update the QoS filter paths in

* {@link #addQosFiltering} whenever a new jersey resource is added here.

kfaraz · 2025-05-26T07:54:22Z

+          },
+          threadsForOvelordWork
+      );
+      JettyServerInitUtils.addFilters(root, Collections.singleton(filterHolder));


If already bound using JettyBindings.addQosFilter in addOverlordJerseyResources, we wouldn't need to call this at all since JettyServerInitUtils.addQosFilters is already being called from OverlordJettyServerInitializer.initialize.

kfaraz · 2025-05-26T07:57:06Z

+      JettyBindings.QosFilterHolder filterHolder = new JettyBindings.QosFilterHolder(
+          new String[]{
+              "/druid-internal/v1/*",
+              "/druid/indexer/v1/*"
+          },
+          threadsForOvelordWork
+      );


Other modules seem to use JettyBindings.addQosFilter for this purpose. Please see comment on addOverlordJerseyResources method.

kfaraz · 2025-05-26T07:59:07Z

+          "QOS filter is disabled for the overlord requests." +
+          "Set `druid.server.http.numThread` to a value greater than %d to enable QoSFilter.",
+          ServerConfig.DEFAULT_NUM_PACKING_THREADS


Suggested change

"QOS filter is disabled for the overlord requests." +

"Set `druid.server.http.numThread` to a value greater than %d to enable QoSFilter.",

ServerConfig.DEFAULT_NUM_PACKING_THREADS

"QoS filtering is disabled for Overlord requests. " +

"Set `druid.server.http.numThreads` to a value greater than [%d] to enable.",

ServerConfig.DEFAULT_NUM_PACKING_THREADS

Thanks for the catch.

kfaraz · 2025-05-26T07:59:27Z

+  protected static boolean addQOSFiltering(ServletContextHandler root, int threadsForOvelordWork)
+  {
+    if (threadsForOvelordWork >= ServerConfig.DEFAULT_NUM_PACKING_THREADS) {
+      log.info("Enabling QOS filter on overlord requests with limit [%d].", threadsForOvelordWork);


Suggested change

log.info("Enabling QOS filter on overlord requests with limit [%d].", threadsForOvelordWork);

log.info("Enabling QoS filtering for Overlord requests with limit[%d].", threadsForOvelordWork);

kfaraz · 2025-05-26T08:04:45Z

+   * {@link org.apache.druid.cli.CliOverlord#addQOSFiltering(ServletContextHandler, int)} when a new jersey resource is added.
   */
-  private static class OverlordJettyServerInitializer implements JettyServerInitializer
+  private static void addOverlordJerseyResources(Binder binder)


Going by what other modules such as LookupModule are doing, we should bind the qos filter in this method itself using JettyBindings.addQosFilter.

Since we donot have the exact thread pool handy, at the time of binding we can probably not do this.

Okay Found a way to do this. Adjusting.

github-actions · 2025-07-27T00:29:30Z

This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.

cryptoe · 2025-07-27T07:10:07Z

Will get to this soon.

github-actions · 2025-09-27T00:22:16Z

This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.

github-actions · 2025-10-25T00:23:14Z

This pull request/issue has been closed due to lack of activity. If you think that
is incorrect, or the pull request requires review, you can revive the PR at any time.

cryptoe · 2025-11-17T10:30:07Z

reopen

gianm · 2025-11-18T07:14:31Z

+
+            final int threadsForOverlordWork = serverHttpNumThreads - THREADS_RESERVED_FOR_HEALTH_CHECK;
+
+            if (threadsForOverlordWork >= ServerConfig.DEFAULT_MIN_QOS_THRESHOLD) {


Why is this based on ServerConfig.DEFAULT_MIN_QOS_THRESHOLD (30)? If someone sets druid.server.http.numThreads = 25 then they won't have QoS; I don't understand why that is good.

Secondly, IMO it would be better to apply the QoS to the action API only. That way, if the OL is bogged down with handling actions, it can still respond to the APIs that the web console uses. Applying QoS to all OL APIs, like this PR currently does, would make it able to respond to health checks but not actually function in any other way. It seems to defeat the purpose of a health check.

So, how about having a config like druid.indexer.server.maxConcurrentActions where the default is based on serverHttpNumThreads? Perhaps max(1, serverHttpNumThreads - 4) or max(1, serverHttpNumThreads * 0.8). And allow admins to set it to druid.indexer.server.maxConcurrentActions = 0 which would disable the QoS.

I was optimizing the patch for larger clusters, because that was the use case I had at that time, but thinking more you are correct it should happen across the board.

Added a new config:
"druid.indexer.server.maxConcurrentActions"

Went with max(1, max(serverHttpNumThreads - 4, serverHttpNumThreads * 0.8)) so that large clusters are not impacted much.

+
+
+            final int serverHttpNumThreads = properties.containsKey(CliIndexerServerModule.SERVER_HTTP_NUM_THREADS_PROPERTY)
+                                             ? Integer.parseInt(properties.getProperty(CliIndexerServerModule.SERVER_HTTP_NUM_THREADS_PROPERTY))


+
+            final int maxConcurrentActions;
+            if (properties.containsKey("druid.indexer.server.maxConcurrentActions")) {
+              maxConcurrentActions = Integer.parseInt(properties.getProperty("druid.indexer.server.maxConcurrentActions"));


Adding QOSFiltering for health checks on the overlord.

d7aa580

cryptoe requested a review from kfaraz May 26, 2025 07:10

Adding QOSFiltering for health checks on the overlord.

45deabe

cryptoe force-pushed the overlord_heath_checks branch from 107aadb to 45deabe Compare May 26, 2025 07:13

github-advanced-security AI found potential problems May 26, 2025

View reviewed changes

Comment thread services/src/main/java/org/apache/druid/cli/CliOverlord.java Fixed

Comment thread services/src/test/java/org/apache/druid/cli/CliOverlordTest.java Fixed

kfaraz reviewed May 26, 2025

View reviewed changes

github-actions Bot added the stale label Jul 27, 2025

github-actions Bot removed the stale label Jul 28, 2025

github-actions Bot added the stale label Sep 27, 2025

github-actions Bot closed this Oct 25, 2025

Pushing latest changes.

cee2e7b

cryptoe reopened this Nov 17, 2025

cryptoe added 8 commits November 17, 2025 16:19

Fixing compile

8a39249

Guice way of doing things

c32b9da

Review comments

986c93b

Remove extra line

83cce04

Lesser code

ad2dbd0

even smaller diff

60a26a1

Adjusting more

3907700

checkstyle fixes

f1fe5ae

github-advanced-security AI found potential problems Nov 17, 2025

View reviewed changes

Comment thread server/src/main/java/org/apache/druid/server/initialization/ServerConfig.java Fixed

Comment thread services/src/main/java/org/apache/druid/cli/CliOverlord.java Fixed

github-actions Bot removed the stale label Nov 18, 2025

gianm reviewed Nov 18, 2025

View reviewed changes

Addressing review comments.

0b3cff8

github-actions Bot added the Area - Documentation label Nov 19, 2025

github-advanced-security AI found potential problems Nov 19, 2025

View reviewed changes

Fix spell check

c5b84dc

gianm approved these changes Nov 19, 2025

View reviewed changes

gianm merged commit 713be4a into apache:master Nov 19, 2025
57 checks passed

kgyrtkirk added this to the 36.0.0 milestone Jan 19, 2026

cryptoe mentioned this pull request Apr 7, 2026

minor: Add QOS filtering for coordinator endpoints #19271

Open

		final QueuedThreadPool queuedThreadPool = (QueuedThreadPool) server.getThreadPool();
		final int maxThreads = queuedThreadPool.getMaxThreads();

	protected static boolean addQOSFiltering(ServletContextHandler root, int threadsForOvelordWork)
	protected static boolean addQosFiltering(ServletContextHandler root, int threadsForOvelordWork)

		* As QOS filtering is enabled on overlord requests, we need to update the QOS filter paths in
		* {@link org.apache.druid.cli.CliOverlord#addQOSFiltering(ServletContextHandler, int)} when a new jersey resource is added.

	log.info("Enabling QOS filter on overlord requests with limit [%d].", threadsForOvelordWork);
	log.info("Enabling QoS filtering for Overlord requests with limit[%d].", threadsForOvelordWork);


		final int threadsForOverlordWork = serverHttpNumThreads - THREADS_RESERVED_FOR_HEALTH_CHECK;

		if (threadsForOverlordWork >= ServerConfig.DEFAULT_MIN_QOS_THRESHOLD) {



		final int serverHttpNumThreads = properties.containsKey(CliIndexerServerModule.SERVER_HTTP_NUM_THREADS_PROPERTY)
		? Integer.parseInt(properties.getProperty(CliIndexerServerModule.SERVER_HTTP_NUM_THREADS_PROPERTY))

Conversation

cryptoe commented May 26, 2025

Uh oh!

Uh oh!

Uh oh!

kfaraz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cryptoe May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 27, 2025

Uh oh!

cryptoe commented Jul 27, 2025

Uh oh!

github-actions Bot commented Sep 27, 2025

Uh oh!

github-actions Bot commented Oct 25, 2025

Uh oh!

cryptoe commented Nov 17, 2025

Uh oh!

Uh oh!

Uh oh!

gianm Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cryptoe Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Check notice

Check notice

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cryptoe May 27, 2025 •

edited

Loading

gianm Nov 18, 2025 •

edited

Loading

cryptoe Nov 19, 2025 •

edited

Loading