You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "cryptoe (via GitHub)" <gi...@apache.org> on 2025/11/17 10:30:10 UTC
[PR] Add QOS filtering of overlord requests (druid)
cryptoe opened a new pull request, #18033:
URL: https://github.com/apache/druid/pull/18033
Adds QOS filtering for the overlord so that health check threads are not blocked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #18033:
URL: https://github.com/apache/druid/pull/18033#discussion_r2108888824
##########
server/src/main/java/org/apache/druid/server/initialization/jetty/JettyServerModule.java:
##########
@@ -196,24 +196,23 @@ static Server makeAndInitializeServer(
// that concurrently handle the requests".
int numServerThreads = config.getNumThreads() + getMaxJettyAcceptorsSelectorsNum(node);
- final QueuedThreadPool threadPool;
if (config.getQueueSize() == Integer.MAX_VALUE) {
- threadPool = new QueuedThreadPool();
- threadPool.setMinThreads(numServerThreads);
- threadPool.setMaxThreads(numServerThreads);
+ jettyServerThreadPool = new QueuedThreadPool();
+ jettyServerThreadPool.setMinThreads(numServerThreads);
+ jettyServerThreadPool.setMaxThreads(numServerThreads);
} else {
- threadPool = new QueuedThreadPool(
+ jettyServerThreadPool = new QueuedThreadPool(
numServerThreads,
numServerThreads,
60000, // same default is used in other case when threadPool = new QueuedThreadPool()
new LinkedBlockingQueue<>(config.getQueueSize())
);
}
- threadPool.setDaemon(true);
- jettyServerThreadPool = threadPool;
+ jettyServerThreadPool.setDaemon(true);
Review Comment:
Fixed the jettyServerPool to get initialized in one go with the relevant metric name.
The deamon stuff should not effect the monitor emitting metrics.
Yeah there is a race here. Agreed we can fix it as part of another PR.
##########
server/src/main/java/org/apache/druid/server/initialization/jetty/JettyServerModule.java:
##########
@@ -196,24 +196,23 @@ static Server makeAndInitializeServer(
// that concurrently handle the requests".
int numServerThreads = config.getNumThreads() + getMaxJettyAcceptorsSelectorsNum(node);
- final QueuedThreadPool threadPool;
if (config.getQueueSize() == Integer.MAX_VALUE) {
- threadPool = new QueuedThreadPool();
- threadPool.setMinThreads(numServerThreads);
- threadPool.setMaxThreads(numServerThreads);
+ jettyServerThreadPool = new QueuedThreadPool();
+ jettyServerThreadPool.setMinThreads(numServerThreads);
+ jettyServerThreadPool.setMaxThreads(numServerThreads);
} else {
- threadPool = new QueuedThreadPool(
+ jettyServerThreadPool = new QueuedThreadPool(
numServerThreads,
numServerThreads,
60000, // same default is used in other case when threadPool = new QueuedThreadPool()
new LinkedBlockingQueue<>(config.getQueueSize())
);
}
- threadPool.setDaemon(true);
- jettyServerThreadPool = threadPool;
+ jettyServerThreadPool.setDaemon(true);
Review Comment:
Fixed the jettyServerPool to get initialized in one go with the relevant threads.
The deamon stuff should not effect the monitor emitting metrics.
Yeah there is a race here. Agreed we can fix it as part of another PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #18033:
URL: https://github.com/apache/druid/pull/18033#issuecomment-3340862415
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18033:
URL: https://github.com/apache/druid/pull/18033#discussion_r2108933433
##########
server/src/main/java/org/apache/druid/server/initialization/jetty/JettyServerModule.java:
##########
@@ -196,24 +196,23 @@ static Server makeAndInitializeServer(
// that concurrently handle the requests".
int numServerThreads = config.getNumThreads() + getMaxJettyAcceptorsSelectorsNum(node);
- final QueuedThreadPool threadPool;
if (config.getQueueSize() == Integer.MAX_VALUE) {
- threadPool = new QueuedThreadPool();
- threadPool.setMinThreads(numServerThreads);
- threadPool.setMaxThreads(numServerThreads);
+ jettyServerThreadPool = new QueuedThreadPool();
+ jettyServerThreadPool.setMinThreads(numServerThreads);
+ jettyServerThreadPool.setMaxThreads(numServerThreads);
} else {
- threadPool = new QueuedThreadPool(
+ jettyServerThreadPool = new QueuedThreadPool(
numServerThreads,
numServerThreads,
60000, // same default is used in other case when threadPool = new QueuedThreadPool()
new LinkedBlockingQueue<>(config.getQueueSize())
);
}
- threadPool.setDaemon(true);
- jettyServerThreadPool = threadPool;
+ jettyServerThreadPool.setDaemon(true);
Review Comment:
Are the changes to this file really needed in this PR?
I am not sure how it affects the main QoS change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #18033:
URL: https://github.com/apache/druid/pull/18033#discussion_r2106752994
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -542,4 +570,27 @@
server.setHandler(handlerList);
}
}
+
+ 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);
+ JettyBindings.QosFilterHolder filterHolder = new JettyBindings.QosFilterHolder(
+ new String[]{
+ "/druid-internal/v1/*",
+ "/druid/indexer/v1/*"
+ },
+ threadsForOvelordWork
+ );
+ JettyServerInitUtils.addFilters(root, Collections.singleton(filterHolder));
+ return true;
+ } else {
+ log.info(
+ "QOS filter is disabled for the overlord requests." +
+ "Set `druid.server.http.numThread` to a value greater than %d to enable QoSFilter.",
Review Comment:
## Missing space in string literal
This string appears to be missing a space after 'requests.'.
[Show more details](https://github.com/apache/druid/security/code-scanning/9198)
##########
services/src/test/java/org/apache/druid/cli/CliOverlordTest.java:
##########
@@ -47,4 +48,32 @@
= overlordInjector.getInstance(SegmentsMetadataManager.class);
Assert.assertTrue(segmentsMetadataManager instanceof SqlSegmentsMetadataManagerV2);
}
+
+ @Test
+ public void testQosFilteringEnabled()
+ {
+ ServletContextHandler handler = new ServletContextHandler();
+ final int threadsForOverlordWork = 30;
+
+ Assert.assertTrue(CliOverlord.addQOSFiltering(handler, threadsForOverlordWork));
+
+ Assert.assertEquals(
+ threadsForOverlordWork,
+ Integer.parseInt(handler.getServletHandler().getFilters()[0].getInitParameters().get("maxRequests"))
Review Comment:
## Missing catch of NumberFormatException
Potential uncaught 'java.lang.NumberFormatException'.
[Show more details](https://github.com/apache/druid/security/code-scanning/9197)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #18033:
URL: https://github.com/apache/druid/pull/18033#issuecomment-3123636641
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #18033:
URL: https://github.com/apache/druid/pull/18033#discussion_r2108877577
##########
server/src/main/java/org/apache/druid/server/initialization/ServerConfig.java:
##########
@@ -46,6 +46,7 @@
public class ServerConfig
{
public static final int DEFAULT_GZIP_INFLATE_BUFFER_SIZE = 4096;
+ public static final int DEFAULT_NUM_PACKING_THREADS = 30;
Review Comment:
its the extra threads we add . Couldn't really find a good word.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm merged PR #18033:
URL: https://github.com/apache/druid/pull/18033
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #18033:
URL: https://github.com/apache/druid/pull/18033#issuecomment-3445314306
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #18033:
URL: https://github.com/apache/druid/pull/18033#discussion_r2542044293
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -458,6 +455,47 @@ private void configureOverlordHelpers(Binder binder)
dutyBinder.addBinding().to(TaskLogAutoCleaner.class);
dutyBinder.addBinding().to(UnusedSegmentsKiller.class).in(LazySingleton.class);
}
+
+ /**
+ * Configures Overlord-specific web resources and QoS filtering.
+ * This method performs two main tasks:
+ * <ol>
+ * <li>Registers Jersey resources for Overlord REST endpoints</li>
+ * <li>Configures QoS (Quality of Service) filtering for request limiting</li>
+ * </ol>
+ * <p>
+ * The Jersey resources handle the following endpoint paths:
+ * <ul>
+ * <li>/druid/indexer/v1 - Main indexing and task management endpoints</li>
+ * <li>/druid-internal/v1 - Internal Overlord management endpoints</li>
+ * </ul>
+ * Note to developers:
+ * Whenever adding new resources, please check if the root paths are added in the QOS filtering.
+ */
+ private void configureOverlordWebResources(Binder binder)
+ {
+ Jerseys.addResource(binder, OverlordResource.class);
+ Jerseys.addResource(binder, SupervisorResource.class);
+ Jerseys.addResource(binder, HttpRemoteTaskRunnerResource.class);
+ Jerseys.addResource(binder, OverlordCompactionResource.class);
+ Jerseys.addResource(binder, OverlordDataSourcesResource.class);
+
+ // Add QoS filtering for overlord-specific endpoints if we have enough threads
+ final int serverHttpNumThreads = properties.containsKey("druid.server.http.numThreads")
+ ? Integer.parseInt(properties.getProperty("druid.server.http.numThreads"))
+ : ServerConfig.getDefaultNumThreads();
+
+ final int threadsForOverlordWork = serverHttpNumThreads - THREADS_RESERVED_FOR_HEALTH_CHECK;
+
+ if (threadsForOverlordWork >= ServerConfig.DEFAULT_MIN_QOS_THRESHOLD) {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18033:
URL: https://github.com/apache/druid/pull/18033#discussion_r2106735032
##########
server/src/main/java/org/apache/druid/server/initialization/ServerConfig.java:
##########
@@ -46,6 +46,7 @@
public class ServerConfig
{
public static final int DEFAULT_GZIP_INFLATE_BUFFER_SIZE = 4096;
+ public static final int DEFAULT_NUM_PACKING_THREADS = 30;
Review Comment:
What does "packing" signify here?
##########
server/src/main/java/org/apache/druid/server/initialization/jetty/JettyServerModule.java:
##########
@@ -196,24 +196,23 @@ static Server makeAndInitializeServer(
// that concurrently handle the requests".
int numServerThreads = config.getNumThreads() + getMaxJettyAcceptorsSelectorsNum(node);
- final QueuedThreadPool threadPool;
if (config.getQueueSize() == Integer.MAX_VALUE) {
- threadPool = new QueuedThreadPool();
- threadPool.setMinThreads(numServerThreads);
- threadPool.setMaxThreads(numServerThreads);
+ jettyServerThreadPool = new QueuedThreadPool();
+ jettyServerThreadPool.setMinThreads(numServerThreads);
+ jettyServerThreadPool.setMaxThreads(numServerThreads);
} else {
- threadPool = new QueuedThreadPool(
+ jettyServerThreadPool = new QueuedThreadPool(
numServerThreads,
numServerThreads,
60000, // same default is used in other case when threadPool = new QueuedThreadPool()
new LinkedBlockingQueue<>(config.getQueueSize())
);
}
- threadPool.setDaemon(true);
- jettyServerThreadPool = threadPool;
+ jettyServerThreadPool.setDaemon(true);
Review Comment:
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.
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -542,4 +570,27 @@ public void initialize(Server server, Injector injector)
server.setHandler(handlerList);
}
}
+
+ protected static boolean addQOSFiltering(ServletContextHandler root, int threadsForOvelordWork)
Review Comment:
Please add a short javadoc.
```suggestion
protected static boolean addQosFiltering(ServletContextHandler root, int threadsForOvelordWork)
```
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -542,4 +570,27 @@ public void initialize(Server server, Injector injector)
server.setHandler(handlerList);
}
}
+
+ 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);
+ JettyBindings.QosFilterHolder filterHolder = new JettyBindings.QosFilterHolder(
+ new String[]{
+ "/druid-internal/v1/*",
+ "/druid/indexer/v1/*"
+ },
+ threadsForOvelordWork
+ );
+ JettyServerInitUtils.addFilters(root, Collections.singleton(filterHolder));
Review Comment:
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`.
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -487,6 +508,13 @@ public void initialize(Server server, Injector injector)
final ObjectMapper jsonMapper = injector.getInstance(Key.get(ObjectMapper.class, Json.class));
final AuthenticatorMapper authenticatorMapper = injector.getInstance(AuthenticatorMapper.class);
+ final QueuedThreadPool queuedThreadPool = (QueuedThreadPool) server.getThreadPool();
+ final int maxThreads = queuedThreadPool.getMaxThreads();
Review Comment:
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.
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -487,6 +508,13 @@ public void initialize(Server server, Injector injector)
final ObjectMapper jsonMapper = injector.getInstance(Key.get(ObjectMapper.class, Json.class));
final AuthenticatorMapper authenticatorMapper = injector.getInstance(AuthenticatorMapper.class);
+ final QueuedThreadPool queuedThreadPool = (QueuedThreadPool) server.getThreadPool();
+ final int maxThreads = queuedThreadPool.getMaxThreads();
+ final int threadsForOvelordWork = maxThreads - THREADS_RESERVED_FOR_HEALTH_CHECK;
Review Comment:
Nit: we can inline this variable in the method call.
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -460,9 +460,30 @@ private void configureOverlordHelpers(Binder binder)
);
}
+
/**
+ * Currenlty, the resource paths of the jersery resources on the overlord start with
+ * <ol>
+ * <li>/druid/indexer/v1</li>
+ * <li>/druid-internal/v1</li>
+ * </ol>
+ * <p>
+ * 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.
Review Comment:
```suggestion
* Since QoS filtering is enabled for Overlord requests, update the QoS filter paths in
* {@link #addQosFiltering} whenever a new jersey resource is added here.
```
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -542,4 +570,27 @@ public void initialize(Server server, Injector injector)
server.setHandler(handlerList);
}
}
+
+ 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);
+ JettyBindings.QosFilterHolder filterHolder = new JettyBindings.QosFilterHolder(
+ new String[]{
+ "/druid-internal/v1/*",
+ "/druid/indexer/v1/*"
+ },
+ threadsForOvelordWork
+ );
+ JettyServerInitUtils.addFilters(root, Collections.singleton(filterHolder));
+ return true;
+ } else {
+ log.info(
+ "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
Review Comment:
```suggestion
"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
```
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -542,4 +570,27 @@ public void initialize(Server server, Injector injector)
server.setHandler(handlerList);
}
}
+
+ 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);
Review Comment:
```suggestion
log.info("Enabling QoS filtering for Overlord requests with limit[%d].", threadsForOvelordWork);
```
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -542,4 +570,27 @@ public void initialize(Server server, Injector injector)
server.setHandler(handlerList);
}
}
+
+ protected static boolean addQOSFiltering(ServletContextHandler root, int threadsForOvelordWork)
Review Comment:
Nit: rename the second arg to `processingThreads` or something similar.
```suggestion
protected static boolean addQOSFiltering(ServletContextHandler root, int threadsForOvelordWork)
```
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -460,9 +460,30 @@ private void configureOverlordHelpers(Binder binder)
);
}
+
/**
+ * Currenlty, the resource paths of the jersery resources on the overlord start with
+ * <ol>
+ * <li>/druid/indexer/v1</li>
+ * <li>/druid-internal/v1</li>
+ * </ol>
+ * <p>
+ * 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.
*/
- private static class OverlordJettyServerInitializer implements JettyServerInitializer
+ private static void addOverlordJerseyResources(Binder binder)
Review Comment:
Going by what other modules such as `LookupModule` are doing, we should bind the qos filter in this method itself using `JettyBindings.addQosFilter`.
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -542,4 +570,27 @@ public void initialize(Server server, Injector injector)
server.setHandler(handlerList);
}
}
+
+ 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);
+ JettyBindings.QosFilterHolder filterHolder = new JettyBindings.QosFilterHolder(
+ new String[]{
+ "/druid-internal/v1/*",
+ "/druid/indexer/v1/*"
+ },
+ threadsForOvelordWork
+ );
Review Comment:
Other modules seem to use `JettyBindings.addQosFilter` for this purpose. Please see comment on `addOverlordJerseyResources` method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #18033:
URL: https://github.com/apache/druid/pull/18033#discussion_r2533794052
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -458,6 +455,47 @@
dutyBinder.addBinding().to(TaskLogAutoCleaner.class);
dutyBinder.addBinding().to(UnusedSegmentsKiller.class).in(LazySingleton.class);
}
+
+ /**
+ * Configures Overlord-specific web resources and QoS filtering.
+ * This method performs two main tasks:
+ * <ol>
+ * <li>Registers Jersey resources for Overlord REST endpoints</li>
+ * <li>Configures QoS (Quality of Service) filtering for request limiting</li>
+ * </ol>
+ * <p>
+ * The Jersey resources handle the following endpoint paths:
+ * <ul>
+ * <li>/druid/indexer/v1 - Main indexing and task management endpoints</li>
+ * <li>/druid-internal/v1 - Internal Overlord management endpoints</li>
+ * </ul>
+ * Note to developers:
+ * Whenever adding new resources, please check if the root paths are added in the QOS filtering.
+ */
+ private void configureOverlordWebResources(Binder binder)
+ {
+ Jerseys.addResource(binder, OverlordResource.class);
+ Jerseys.addResource(binder, SupervisorResource.class);
+ Jerseys.addResource(binder, HttpRemoteTaskRunnerResource.class);
+ Jerseys.addResource(binder, OverlordCompactionResource.class);
+ Jerseys.addResource(binder, OverlordDataSourcesResource.class);
+
+ // Add QoS filtering for overlord-specific endpoints if we have enough threads
+ final int serverHttpNumThreads = properties.containsKey("druid.server.http.numThreads")
+ ? Integer.parseInt(properties.getProperty("druid.server.http.numThreads"))
Review Comment:
## Missing catch of NumberFormatException
Potential uncaught 'java.lang.NumberFormatException'.
[Show more details](https://github.com/apache/druid/security/code-scanning/10555)
##########
server/src/main/java/org/apache/druid/server/initialization/ServerConfig.java:
##########
@@ -438,7 +439,7 @@
public static int getDefaultNumThreads()
{
- return Math.max(10, (JvmUtils.getRuntimeInfo().getAvailableProcessors() * 17) / 16 + 2) + 30;
+ return Math.max(10, (JvmUtils.getRuntimeInfo().getAvailableProcessors() * 17) / 16 + 2) + DEFAULT_MIN_QOS_THRESHOLD;
Review Comment:
## Deprecated method or constructor invocation
Invoking [JvmUtils.getRuntimeInfo](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/10556)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #18033:
URL: https://github.com/apache/druid/pull/18033#discussion_r2542044293
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -458,6 +455,47 @@ private void configureOverlordHelpers(Binder binder)
dutyBinder.addBinding().to(TaskLogAutoCleaner.class);
dutyBinder.addBinding().to(UnusedSegmentsKiller.class).in(LazySingleton.class);
}
+
+ /**
+ * Configures Overlord-specific web resources and QoS filtering.
+ * This method performs two main tasks:
+ * <ol>
+ * <li>Registers Jersey resources for Overlord REST endpoints</li>
+ * <li>Configures QoS (Quality of Service) filtering for request limiting</li>
+ * </ol>
+ * <p>
+ * The Jersey resources handle the following endpoint paths:
+ * <ul>
+ * <li>/druid/indexer/v1 - Main indexing and task management endpoints</li>
+ * <li>/druid-internal/v1 - Internal Overlord management endpoints</li>
+ * </ul>
+ * Note to developers:
+ * Whenever adding new resources, please check if the root paths are added in the QOS filtering.
+ */
+ private void configureOverlordWebResources(Binder binder)
+ {
+ Jerseys.addResource(binder, OverlordResource.class);
+ Jerseys.addResource(binder, SupervisorResource.class);
+ Jerseys.addResource(binder, HttpRemoteTaskRunnerResource.class);
+ Jerseys.addResource(binder, OverlordCompactionResource.class);
+ Jerseys.addResource(binder, OverlordDataSourcesResource.class);
+
+ // Add QoS filtering for overlord-specific endpoints if we have enough threads
+ final int serverHttpNumThreads = properties.containsKey("druid.server.http.numThreads")
+ ? Integer.parseInt(properties.getProperty("druid.server.http.numThreads"))
+ : ServerConfig.getDefaultNumThreads();
+
+ final int threadsForOverlordWork = serverHttpNumThreads - THREADS_RESERVED_FOR_HEALTH_CHECK;
+
+ if (threadsForOverlordWork >= ServerConfig.DEFAULT_MIN_QOS_THRESHOLD) {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #18033:
URL: https://github.com/apache/druid/pull/18033#discussion_r2108888824
##########
server/src/main/java/org/apache/druid/server/initialization/jetty/JettyServerModule.java:
##########
@@ -196,24 +196,23 @@ static Server makeAndInitializeServer(
// that concurrently handle the requests".
int numServerThreads = config.getNumThreads() + getMaxJettyAcceptorsSelectorsNum(node);
- final QueuedThreadPool threadPool;
if (config.getQueueSize() == Integer.MAX_VALUE) {
- threadPool = new QueuedThreadPool();
- threadPool.setMinThreads(numServerThreads);
- threadPool.setMaxThreads(numServerThreads);
+ jettyServerThreadPool = new QueuedThreadPool();
+ jettyServerThreadPool.setMinThreads(numServerThreads);
+ jettyServerThreadPool.setMaxThreads(numServerThreads);
} else {
- threadPool = new QueuedThreadPool(
+ jettyServerThreadPool = new QueuedThreadPool(
numServerThreads,
numServerThreads,
60000, // same default is used in other case when threadPool = new QueuedThreadPool()
new LinkedBlockingQueue<>(config.getQueueSize())
);
}
- threadPool.setDaemon(true);
- jettyServerThreadPool = threadPool;
+ jettyServerThreadPool.setDaemon(true);
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #18033:
URL: https://github.com/apache/druid/pull/18033#discussion_r2542166183
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -458,6 +455,50 @@
dutyBinder.addBinding().to(TaskLogAutoCleaner.class);
dutyBinder.addBinding().to(UnusedSegmentsKiller.class).in(LazySingleton.class);
}
+
+ /**
+ * Configures Overlord-specific web resources and QoS filtering.
+ * This method performs two main tasks:
+ * <ol>
+ * <li>Registers Jersey resources for Overlord REST endpoints</li>
+ * <li>Configures QoS (Quality of Service) filtering for action APIs only</li>
+ * </ol>
+ * <p>
+ * QoS filtering is applied to action APIs to prevent the Overlord from becoming
+ * unresponsive to health checks
+ */
+ private void configureOverlordWebResources(Binder binder)
+ {
+ Jerseys.addResource(binder, OverlordResource.class);
+ Jerseys.addResource(binder, SupervisorResource.class);
+ Jerseys.addResource(binder, HttpRemoteTaskRunnerResource.class);
+ Jerseys.addResource(binder, OverlordCompactionResource.class);
+ Jerseys.addResource(binder, OverlordDataSourcesResource.class);
+
+
+ final int serverHttpNumThreads = properties.containsKey(CliIndexerServerModule.SERVER_HTTP_NUM_THREADS_PROPERTY)
+ ? Integer.parseInt(properties.getProperty(CliIndexerServerModule.SERVER_HTTP_NUM_THREADS_PROPERTY))
Review Comment:
## Missing catch of NumberFormatException
Potential uncaught 'java.lang.NumberFormatException'.
[Show more details](https://github.com/apache/druid/security/code-scanning/10563)
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -458,6 +455,50 @@
dutyBinder.addBinding().to(TaskLogAutoCleaner.class);
dutyBinder.addBinding().to(UnusedSegmentsKiller.class).in(LazySingleton.class);
}
+
+ /**
+ * Configures Overlord-specific web resources and QoS filtering.
+ * This method performs two main tasks:
+ * <ol>
+ * <li>Registers Jersey resources for Overlord REST endpoints</li>
+ * <li>Configures QoS (Quality of Service) filtering for action APIs only</li>
+ * </ol>
+ * <p>
+ * QoS filtering is applied to action APIs to prevent the Overlord from becoming
+ * unresponsive to health checks
+ */
+ private void configureOverlordWebResources(Binder binder)
+ {
+ Jerseys.addResource(binder, OverlordResource.class);
+ Jerseys.addResource(binder, SupervisorResource.class);
+ Jerseys.addResource(binder, HttpRemoteTaskRunnerResource.class);
+ Jerseys.addResource(binder, OverlordCompactionResource.class);
+ Jerseys.addResource(binder, OverlordDataSourcesResource.class);
+
+
+ final int serverHttpNumThreads = properties.containsKey(CliIndexerServerModule.SERVER_HTTP_NUM_THREADS_PROPERTY)
+ ? Integer.parseInt(properties.getProperty(CliIndexerServerModule.SERVER_HTTP_NUM_THREADS_PROPERTY))
+ : ServerConfig.getDefaultNumThreads();
+
+ final int maxConcurrentActions;
+ if (properties.containsKey("druid.indexer.server.maxConcurrentActions")) {
+ maxConcurrentActions = Integer.parseInt(properties.getProperty("druid.indexer.server.maxConcurrentActions"));
Review Comment:
## Missing catch of NumberFormatException
Potential uncaught 'java.lang.NumberFormatException'.
[Show more details](https://github.com/apache/druid/security/code-scanning/10564)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #18033:
URL: https://github.com/apache/druid/pull/18033#discussion_r2536599057
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -458,6 +455,47 @@ private void configureOverlordHelpers(Binder binder)
dutyBinder.addBinding().to(TaskLogAutoCleaner.class);
dutyBinder.addBinding().to(UnusedSegmentsKiller.class).in(LazySingleton.class);
}
+
+ /**
+ * Configures Overlord-specific web resources and QoS filtering.
+ * This method performs two main tasks:
+ * <ol>
+ * <li>Registers Jersey resources for Overlord REST endpoints</li>
+ * <li>Configures QoS (Quality of Service) filtering for request limiting</li>
+ * </ol>
+ * <p>
+ * The Jersey resources handle the following endpoint paths:
+ * <ul>
+ * <li>/druid/indexer/v1 - Main indexing and task management endpoints</li>
+ * <li>/druid-internal/v1 - Internal Overlord management endpoints</li>
+ * </ul>
+ * Note to developers:
+ * Whenever adding new resources, please check if the root paths are added in the QOS filtering.
+ */
+ private void configureOverlordWebResources(Binder binder)
+ {
+ Jerseys.addResource(binder, OverlordResource.class);
+ Jerseys.addResource(binder, SupervisorResource.class);
+ Jerseys.addResource(binder, HttpRemoteTaskRunnerResource.class);
+ Jerseys.addResource(binder, OverlordCompactionResource.class);
+ Jerseys.addResource(binder, OverlordDataSourcesResource.class);
+
+ // Add QoS filtering for overlord-specific endpoints if we have enough threads
+ final int serverHttpNumThreads = properties.containsKey("druid.server.http.numThreads")
+ ? Integer.parseInt(properties.getProperty("druid.server.http.numThreads"))
+ : ServerConfig.getDefaultNumThreads();
+
+ final int threadsForOverlordWork = serverHttpNumThreads - THREADS_RESERVED_FOR_HEALTH_CHECK;
+
+ if (threadsForOverlordWork >= ServerConfig.DEFAULT_MIN_QOS_THRESHOLD) {
Review Comment:
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 think that makes sense.
Also, 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #18033:
URL: https://github.com/apache/druid/pull/18033#discussion_r2108923316
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -460,9 +460,30 @@ private void configureOverlordHelpers(Binder binder)
);
}
+
/**
+ * Currenlty, the resource paths of the jersery resources on the overlord start with
+ * <ol>
+ * <li>/druid/indexer/v1</li>
+ * <li>/druid-internal/v1</li>
+ * </ol>
+ * <p>
+ * 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.
*/
- private static class OverlordJettyServerInitializer implements JettyServerInitializer
+ private static void addOverlordJerseyResources(Binder binder)
Review Comment:
Since we donot have the exact thread pool handy, at the time of binding we can probably not do this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #18033:
URL: https://github.com/apache/druid/pull/18033#discussion_r2108899505
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -542,4 +570,27 @@ public void initialize(Server server, Injector injector)
server.setHandler(handlerList);
}
}
+
+ 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);
+ JettyBindings.QosFilterHolder filterHolder = new JettyBindings.QosFilterHolder(
+ new String[]{
+ "/druid-internal/v1/*",
+ "/druid/indexer/v1/*"
+ },
+ threadsForOvelordWork
+ );
+ JettyServerInitUtils.addFilters(root, Collections.singleton(filterHolder));
+ return true;
+ } else {
+ log.info(
+ "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
Review Comment:
Thanks for the catch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on PR #18033:
URL: https://github.com/apache/druid/pull/18033#issuecomment-3124140567
Will get to this soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on PR #18033:
URL: https://github.com/apache/druid/pull/18033#issuecomment-3541024417
reopen
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #18033:
URL: https://github.com/apache/druid/pull/18033#discussion_r2536599057
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -458,6 +455,47 @@ private void configureOverlordHelpers(Binder binder)
dutyBinder.addBinding().to(TaskLogAutoCleaner.class);
dutyBinder.addBinding().to(UnusedSegmentsKiller.class).in(LazySingleton.class);
}
+
+ /**
+ * Configures Overlord-specific web resources and QoS filtering.
+ * This method performs two main tasks:
+ * <ol>
+ * <li>Registers Jersey resources for Overlord REST endpoints</li>
+ * <li>Configures QoS (Quality of Service) filtering for request limiting</li>
+ * </ol>
+ * <p>
+ * The Jersey resources handle the following endpoint paths:
+ * <ul>
+ * <li>/druid/indexer/v1 - Main indexing and task management endpoints</li>
+ * <li>/druid-internal/v1 - Internal Overlord management endpoints</li>
+ * </ul>
+ * Note to developers:
+ * Whenever adding new resources, please check if the root paths are added in the QOS filtering.
+ */
+ private void configureOverlordWebResources(Binder binder)
+ {
+ Jerseys.addResource(binder, OverlordResource.class);
+ Jerseys.addResource(binder, SupervisorResource.class);
+ Jerseys.addResource(binder, HttpRemoteTaskRunnerResource.class);
+ Jerseys.addResource(binder, OverlordCompactionResource.class);
+ Jerseys.addResource(binder, OverlordDataSourcesResource.class);
+
+ // Add QoS filtering for overlord-specific endpoints if we have enough threads
+ final int serverHttpNumThreads = properties.containsKey("druid.server.http.numThreads")
+ ? Integer.parseInt(properties.getProperty("druid.server.http.numThreads"))
+ : ServerConfig.getDefaultNumThreads();
+
+ final int threadsForOverlordWork = serverHttpNumThreads - THREADS_RESERVED_FOR_HEALTH_CHECK;
+
+ if (threadsForOverlordWork >= ServerConfig.DEFAULT_MIN_QOS_THRESHOLD) {
Review Comment:
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.
Also, 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.
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -458,6 +455,47 @@ private void configureOverlordHelpers(Binder binder)
dutyBinder.addBinding().to(TaskLogAutoCleaner.class);
dutyBinder.addBinding().to(UnusedSegmentsKiller.class).in(LazySingleton.class);
}
+
+ /**
+ * Configures Overlord-specific web resources and QoS filtering.
+ * This method performs two main tasks:
+ * <ol>
+ * <li>Registers Jersey resources for Overlord REST endpoints</li>
+ * <li>Configures QoS (Quality of Service) filtering for request limiting</li>
+ * </ol>
+ * <p>
+ * The Jersey resources handle the following endpoint paths:
+ * <ul>
+ * <li>/druid/indexer/v1 - Main indexing and task management endpoints</li>
+ * <li>/druid-internal/v1 - Internal Overlord management endpoints</li>
+ * </ul>
+ * Note to developers:
+ * Whenever adding new resources, please check if the root paths are added in the QOS filtering.
+ */
+ private void configureOverlordWebResources(Binder binder)
+ {
+ Jerseys.addResource(binder, OverlordResource.class);
+ Jerseys.addResource(binder, SupervisorResource.class);
+ Jerseys.addResource(binder, HttpRemoteTaskRunnerResource.class);
+ Jerseys.addResource(binder, OverlordCompactionResource.class);
+ Jerseys.addResource(binder, OverlordDataSourcesResource.class);
+
+ // Add QoS filtering for overlord-specific endpoints if we have enough threads
+ final int serverHttpNumThreads = properties.containsKey("druid.server.http.numThreads")
+ ? Integer.parseInt(properties.getProperty("druid.server.http.numThreads"))
+ : ServerConfig.getDefaultNumThreads();
+
+ final int threadsForOverlordWork = serverHttpNumThreads - THREADS_RESERVED_FOR_HEALTH_CHECK;
+
+ if (threadsForOverlordWork >= ServerConfig.DEFAULT_MIN_QOS_THRESHOLD) {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #18033: Add QOS filtering of overlord requests
URL: https://github.com/apache/druid/pull/18033
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add QOS filtering of overlord requests (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #18033:
URL: https://github.com/apache/druid/pull/18033#discussion_r2108929138
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -460,9 +460,30 @@ private void configureOverlordHelpers(Binder binder)
);
}
+
/**
+ * Currenlty, the resource paths of the jersery resources on the overlord start with
+ * <ol>
+ * <li>/druid/indexer/v1</li>
+ * <li>/druid-internal/v1</li>
+ * </ol>
+ * <p>
+ * 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.
*/
- private static class OverlordJettyServerInitializer implements JettyServerInitializer
+ private static void addOverlordJerseyResources(Binder binder)
Review Comment:
Okay Found a way to do this. Adjusting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org