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 2026/04/07 16:44:12 UTC
[PR] Add QOS filtering for coordinator endpoints (druid)
cryptoe opened a new pull request, #19271:
URL: https://github.com/apache/druid/pull/19271
Add QoS filtering to the Coordinator to protect health check endpoints under load
The Coordinator had no request throttling, meaning a burst of slow API requests (segment load status, datasource queries, compaction configs, etc.) could exhaust the Jetty thread pool and make /status/health and /status/ready unresponsive. This is the same problem the Overlord solved in #18033 for its action endpoints.
--
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] minor: Add QOS filtering for coordinator endpoints (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on PR #19271:
URL: https://github.com/apache/druid/pull/19271#issuecomment-5026486734
@capistrant Thank you for taking a look. Addressed your feedback. Please take a look
--
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] minor: Add QOS filtering for coordinator endpoints (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe merged PR #19271:
URL: https://github.com/apache/druid/pull/19271
--
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] minor: Add QOS filtering for coordinator endpoints (druid)
Posted by "capistrant (via GitHub)" <gi...@apache.org>.
capistrant commented on code in PR #19271:
URL: https://github.com/apache/druid/pull/19271#discussion_r3399132362
##########
docs/configuration/index.md:
##########
@@ -746,6 +746,7 @@ These Coordinator static configurations can be defined in the `coordinator/runti
|Property|Description|Default|
|--------|-----------|-------|
|`druid.coordinator.period`|The run period for the Coordinator. The Coordinator operates by maintaining the current state of the world in memory and periodically looking at the set of "used" segments and segments being served to make decisions about whether any changes need to be made to the data topology. This property sets the delay between each of these runs.|`PT60S`|
+|`druid.coordinator.server.maxConcurrentRequests`|Maximum number of concurrent requests to coordinator API endpoints (`/druid/coordinator/v1/*`, `/druid-internal/*`) that the Coordinator will process simultaneously. This prevents thread exhaustion while preserving access to health check endpoints. Set to `-1` to disable quality of service filtering entirely. If not specified, defaults to `max(1, max(serverHttpNumThreads - 4, serverHttpNumThreads * 0.8))`.|`max(1, max(serverHttpNumThreads - 4, serverHttpNumThreads * 0.8))`|
Review Comment:
looks like the code actually gates Qos on `> 0`. Not that it is a big deal but either the doc or the code should reflect this. I think it would be the doc that just says set to `<= 0` to disable?
--
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] minor: Add QOS filtering for coordinator endpoints (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19271:
URL: https://github.com/apache/druid/pull/19271#discussion_r3499366721
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -1347,6 +1460,11 @@ public void tryInit()
try {
recordSupplier = setupRecordSupplier();
+ // Initialize bounded partitions BEFORE first run
+ if (ioConfig.isBounded()) {
Review Comment:
[P1] Bounded-mode calls do not compile
This introduces calls to `ioConfig.isBounded()` and `ioConfig.getBoundedStreamConfig()` plus references to `BoundedStreamConfig`, but `SeekableStreamSupervisorIOConfig` does not define those methods and no `BoundedStreamConfig` type exists in the worktree. The indexing-service module will fail to compile as soon as this file is built; add the config/type and accessors, or remove these bounded-mode paths.
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -4692,6 +5180,25 @@ protected abstract List<SeekableStreamIndexTask<PartitionIdType, SequenceOffsetT
@Nullable List<Integer> serverPrioritiesToAssign
) throws JsonProcessingException;
+ /**
+ * Converts a string representation of a partition ID to the typed partition ID.
+ * Used for deserializing bounded stream config where partition keys come as strings.
+ *
+ * @param partitionIdString string representation of partition ID
+ * @return typed partition ID
+ */
+ protected abstract PartitionIdType createPartitionIdFromString(String partitionIdString);
Review Comment:
[P1] New abstract hooks are not implemented
The base class now declares `createPartitionIdFromString`, `createSequenceOffsetFromObject`, `isOffsetAtOrBeyond`, and `isEndOffsetExclusive` as abstract, but the concrete Kafka/Kinesis supervisors and the seekable-stream test supervisors do not implement them. Those classes will no longer compile unless these hooks get default implementations or all subclasses are updated.
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -3156,11 +3311,64 @@ private boolean updatePartitionDataFromStream()
/**
Review Comment:
[P1] Public hook changes break protected overrides
This changes `getLatestSequencesFromStream` from protected to public; similar public changes were made to `updatePartitionLagFromStream`, `getOffsetsFromMetadataStorage`, and `createDataSourceMetaDataForReset`. Existing overrides in Kafka, Kinesis, and test supervisors are still protected, which Java rejects as reducing visibility. Keep these hooks protected or update every override to public.
--
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] minor: Add QOS filtering for coordinator endpoints (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #19271:
URL: https://github.com/apache/druid/pull/19271#discussion_r3617084070
##########
services/src/main/java/org/apache/druid/cli/CliCoordinator.java:
##########
@@ -229,6 +231,19 @@ public void configure(Binder binder)
binder.bind(JettyServerInitializer.class)
.to(CoordinatorJettyServerInitializer.class);
+ // QoS filtering to prevent heavy coordinator API requests from starving health check endpoints.
+ // Set druid.coordinator.server.maxConcurrentRequests=-1 to disable.
+ final int serverHttpNumThreads = ServerConfig.getNumThreadsFromProperties(properties);
+ final int maxConcurrentRequests = properties.containsKey("druid.coordinator.server.maxConcurrentRequests")
+ ? Integer.parseInt(properties.getProperty("druid.coordinator.server.maxConcurrentRequests"))
+ : ServerConfig.getDefaultMaxConcurrentRequests(serverHttpNumThreads);
+ if (maxConcurrentRequests > 0) {
+ log.info("Coordinator QoS filtering enabled. Max concurrent requests: [%d]", maxConcurrentRequests);
+ JettyBindings.addQosFilter(binder, new String[]{"/druid/coordinator/v1/*", "/druid-internal/*"}, maxConcurrentRequests);
Review Comment:
Done both. Added a PathExclusionQOS filter which felt better than adding a complicated regex matching.
--
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] minor: Add QOS filtering for coordinator endpoints (druid)
Posted by "capistrant (via GitHub)" <gi...@apache.org>.
capistrant commented on code in PR #19271:
URL: https://github.com/apache/druid/pull/19271#discussion_r3623582826
##########
docs/configuration/index.md:
##########
@@ -739,7 +739,7 @@ These Coordinator static configurations can be defined in the `coordinator/runti
|`druid.coordinator.kill.bufferPeriod`|The amount of time that a segment must be unused before it is able to be permanently removed from metadata and deep storage. This can serve as a buffer period to prevent data loss if data ends up being needed after being marked unused.|`P30D`|
|`druid.coordinator.kill.maxSegments`|The number of unused segments to kill per kill task. This number must be greater than 0. This only applies when `druid.coordinator.kill.on=true`.|100|
|`druid.coordinator.kill.maxInterval`|The largest interval, as an [ISO 8601 duration](https://en.wikipedia.org/wiki/ISO_8601#Durations), of segments to delete per kill task. Set to zero, e.g. `PT0S`, for unlimited. This only applies when `druid.coordinator.kill.on=true`.|`P30D`|
-
+|`druid.coordinator.server.maxConcurrentRequests`| Maximum number of concurrent requests to coordinator API endpoints (`/druid/coordinator/v1/*`, `/druid-internal/*`) that the Coordinator will process simultaneously. This prevents thread exhaustion while preserving access to health check endpoints. Set to any value <=0 like `-1` to disable quality of service filtering entirely. If not specified, defaults to `max(1, max(serverHttpNumThreads - 4, serverHttpNumThreads * 0.8))`.|`max(1, max(serverHttpNumThreads - 4, serverHttpNumThreads * 0.8))`|
Review Comment:
suggested re-word to more explicitly state config purpose and discourage operators from lowering value below default (this would be an exceptional circumstance IMO... more harm can be done than good in almost all cases if the value is lowered below default)
```suggestion
|`druid.coordinator.server.maxConcurrentRequests`|Maximum number of requests to non-exempt Coordinator API paths processed concurrently. Requests beyond this are queued briefly and rejected with HTTP 503 if no slot frees. Leadership endpoints (`/leader`, `/isLeader`) and non-matching paths such as `/status/*` are never throttled. This bounds Jetty thread-pool exhaustion from heavy or excessive calls (for example large `/loadstatus`, `/metadata/segments`, or `/intervals` scans). Note: this path also carries recurring internal control-plane traffic (segment handoff checks, historical bootstrap, lookup and rule sync), so setting this too low can throttle that traffic and degrade the cluster; lowering the default is discouraged. Set to any value `<= 0` (for example `-1`) to disable QoS filtering entirely.|`max(1, max(serverHttpNumThreads - 4, serverHttpNumThreads * 0.8))`|
```
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -1456,12 +1456,12 @@ public int registerNewVersionOfPendingSegment(
// from actively reading to pending completion, etc.
// This is a potential silent-loss window where data will not be queryable until handoff.
log.info(
- "Could not find any task matching taskAllocatorId[%s] in supervisor[%s] for upgraded pending segment[%s]"
- + " (upgradedFrom[%s]); it will not be re-announced until handoff.",
- taskAllocatorId,
- supervisorId,
- pendingSegmentRecord.getId(),
- pendingSegmentRecord.getUpgradedFromSegmentId()
+ "Could not find any task matching taskAllocatorId[%s] in supervisor[%s] for upgraded pending segment[%s]"
+ + " (upgradedFrom[%s]); it will not be re-announced until handoff.",
+ taskAllocatorId,
+ supervisorId,
+ pendingSegmentRecord.getId(),
+ pendingSegmentRecord.getUpgradedFromSegmentId()
Review Comment:
unrelated reformatting. if it is fixing legit formatting issue I guess ok to leave, but it is unclear why this file would be being touched for this 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] minor: Add QOS filtering for coordinator endpoints (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on PR #19271:
URL: https://github.com/apache/druid/pull/19271#issuecomment-4318093878
The changes LGTM, no correctness issues found.
--
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] minor: Add QOS filtering for coordinator endpoints (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #19271:
URL: https://github.com/apache/druid/pull/19271#discussion_r3624142186
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -1456,12 +1456,12 @@ public int registerNewVersionOfPendingSegment(
// from actively reading to pending completion, etc.
// This is a potential silent-loss window where data will not be queryable until handoff.
log.info(
- "Could not find any task matching taskAllocatorId[%s] in supervisor[%s] for upgraded pending segment[%s]"
- + " (upgradedFrom[%s]); it will not be re-announced until handoff.",
- taskAllocatorId,
- supervisorId,
- pendingSegmentRecord.getId(),
- pendingSegmentRecord.getUpgradedFromSegmentId()
+ "Could not find any task matching taskAllocatorId[%s] in supervisor[%s] for upgraded pending segment[%s]"
+ + " (upgradedFrom[%s]); it will not be re-announced until handoff.",
+ taskAllocatorId,
+ supervisorId,
+ pendingSegmentRecord.getId(),
+ pendingSegmentRecord.getUpgradedFromSegmentId()
Review Comment:
Its one of those I started changing this file but as I made progress, realized I donot need to touch this file.
The formatting felt off and hence did a reformat of this piece only.
--
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] minor: Add QOS filtering for coordinator endpoints (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #19271:
URL: https://github.com/apache/druid/pull/19271#discussion_r3046711242
##########
server/src/main/java/org/apache/druid/server/initialization/ServerConfig.java:
##########
@@ -441,6 +442,17 @@
return Math.max(10, (JvmUtils.getRuntimeInfo().getAvailableProcessors() * 17) / 16 + 2) + 30;
}
+ public static int getNumThreadsFromProperties(Properties properties)
+ {
+ final String value = properties.getProperty("druid.server.http.numThreads");
+ return value == null ? getDefaultNumThreads() : Integer.parseInt(value);
Review Comment:
## CodeQL / Missing catch of NumberFormatException
Potential uncaught 'java.lang.NumberFormatException'.
[Show more details](https://github.com/apache/druid/security/code-scanning/10956)
##########
services/src/main/java/org/apache/druid/cli/CliCoordinator.java:
##########
@@ -229,6 +231,19 @@
binder.bind(JettyServerInitializer.class)
.to(CoordinatorJettyServerInitializer.class);
+ // QoS filtering to prevent heavy coordinator API requests from starving health check endpoints.
+ // Set druid.coordinator.server.maxConcurrentRequests=-1 to disable.
+ final int serverHttpNumThreads = ServerConfig.getNumThreadsFromProperties(properties);
+ final int maxConcurrentRequests = properties.containsKey("druid.coordinator.server.maxConcurrentRequests")
+ ? Integer.parseInt(properties.getProperty("druid.coordinator.server.maxConcurrentRequests"))
Review Comment:
## CodeQL / Missing catch of NumberFormatException
Potential uncaught 'java.lang.NumberFormatException'.
[Show more details](https://github.com/apache/druid/security/code-scanning/10957)
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -490,27 +489,17 @@
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"));
- } else {
- maxConcurrentActions = getDefaultMaxConcurrentActions(serverHttpNumThreads);
- }
-
+ // QoS filtering to prevent action requests from starving health check endpoints.
+ // Set druid.indexer.server.maxConcurrentActions=-1 to disable.
+ final int serverHttpNumThreads = ServerConfig.getNumThreadsFromProperties(properties);
+ final int maxConcurrentActions = properties.containsKey("druid.indexer.server.maxConcurrentActions")
+ ? Integer.parseInt(properties.getProperty("druid.indexer.server.maxConcurrentActions"))
Review Comment:
## CodeQL / Missing catch of NumberFormatException
Potential uncaught 'java.lang.NumberFormatException'.
[Show more details](https://github.com/apache/druid/security/code-scanning/10958)
--
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] minor: Add QOS filtering for coordinator endpoints (druid)
Posted by "capistrant (via GitHub)" <gi...@apache.org>.
capistrant commented on code in PR #19271:
URL: https://github.com/apache/druid/pull/19271#discussion_r3399210525
##########
services/src/main/java/org/apache/druid/cli/CliCoordinator.java:
##########
@@ -229,6 +231,19 @@ public void configure(Binder binder)
binder.bind(JettyServerInitializer.class)
.to(CoordinatorJettyServerInitializer.class);
+ // QoS filtering to prevent heavy coordinator API requests from starving health check endpoints.
+ // Set druid.coordinator.server.maxConcurrentRequests=-1 to disable.
+ final int serverHttpNumThreads = ServerConfig.getNumThreadsFromProperties(properties);
+ final int maxConcurrentRequests = properties.containsKey("druid.coordinator.server.maxConcurrentRequests")
+ ? Integer.parseInt(properties.getProperty("druid.coordinator.server.maxConcurrentRequests"))
+ : ServerConfig.getDefaultMaxConcurrentRequests(serverHttpNumThreads);
+ if (maxConcurrentRequests > 0) {
+ log.info("Coordinator QoS filtering enabled. Max concurrent requests: [%d]", maxConcurrentRequests);
+ JettyBindings.addQosFilter(binder, new String[]{"/druid/coordinator/v1/*", "/druid-internal/*"}, maxConcurrentRequests);
Review Comment:
could consider writing a CliCoordinatorTest and testing that this binding is added when desired
##########
services/src/main/java/org/apache/druid/cli/CliCoordinator.java:
##########
@@ -229,6 +231,19 @@ public void configure(Binder binder)
binder.bind(JettyServerInitializer.class)
.to(CoordinatorJettyServerInitializer.class);
+ // QoS filtering to prevent heavy coordinator API requests from starving health check endpoints.
+ // Set druid.coordinator.server.maxConcurrentRequests=-1 to disable.
+ final int serverHttpNumThreads = ServerConfig.getNumThreadsFromProperties(properties);
+ final int maxConcurrentRequests = properties.containsKey("druid.coordinator.server.maxConcurrentRequests")
+ ? Integer.parseInt(properties.getProperty("druid.coordinator.server.maxConcurrentRequests"))
+ : ServerConfig.getDefaultMaxConcurrentRequests(serverHttpNumThreads);
+ if (maxConcurrentRequests > 0) {
+ log.info("Coordinator QoS filtering enabled. Max concurrent requests: [%d]", maxConcurrentRequests);
+ JettyBindings.addQosFilter(binder, new String[]{"/druid/coordinator/v1/*", "/druid-internal/*"}, maxConcurrentRequests);
Review Comment:
There are a bunch of light service status endpoints under `/druid/coordinator/v1/*`. found here: https://druid.apache.org/docs/latest/api-reference/service-status-api ... do we really want those blanketed under Qos? I think that `isLeader` could specifically be sensitive to Qos restrictions since monitors/loadBalancers could rely on it. also plain `leader` maybe
--
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