abhishekrb19 commented on code in PR #18148:
URL: https://github.com/apache/druid/pull/18148#discussion_r2412148753
##########
docs/querying/query-context-reference.md:
##########
@@ -44,6 +44,7 @@ Unless otherwise noted, the following parameters apply to all
query types, and t
|Parameter |Default | Description
|
|-------------------|----------------------------------------|----------------------|
|`timeout` | `druid.server.http.defaultQueryTimeout`| Query timeout
in millis, beyond which unfinished queries will be cancelled. 0 timeout means
`no timeout` (up to the server-side maximum query timeout,
`druid.server.http.maxQueryTimeout`). To set the default timeout and maximum
timeout, see [Broker configuration](../configuration/index.md#broker) |
+|`perSegmentTimeout`| `null` | Per-segment
processing timeout in millis, beyond which unfinished queries will be
cancelled. Should be ≤ `timeout`. 0 `perSegmentTimeout` means `no per-segment
timeout`. Generally, a standard default should be O(X seconds). A cluster-wide
default value for this query context can be specified via
`druid.query.default.context.perSegmentTimeout`.|
Review Comment:
I think using -1 as the default to disable per segment timeouts would be
clearer. However, it's consistent with `timeout`'s default strategy, so this is
okay
##########
docs/configuration/index.md:
##########
@@ -1371,6 +1371,7 @@ Processing properties set on the Middle Manager are
passed through to Peons.
|`druid.processing.formatString`|Realtime and Historical processes use this
format string to name their processing threads.|processing-%s|
|`druid.processing.numMergeBuffers`|The number of direct memory buffers
available for merging query results. The buffers are sized by
`druid.processing.buffer.sizeBytes`. This property is effectively a concurrency
limit for queries that require merging buffers. If you are using any queries
that require merge buffers (currently, just groupBy) then you should have at
least two of these.|`max(2, druid.processing.numThreads / 4)`|
|`druid.processing.numThreads`|The number of processing threads to have
available for parallel processing of segments. Our rule of thumb is `num_cores
- 1`, which means that even under heavy load there will still be one core
available to do background tasks like talking with ZooKeeper and pulling down
segments. If only one core is available, this property defaults to the value
`1`.|Number of cores - 1 (or 1)|
+|`druid.processing.numTimeoutThreads`|The number of processing threads to have
available for handling per-segment query timeouts. Setting this value to `0`
removes the ability to service per-segment timeouts, irrespective of
query-level configs. As these threads are just servicing timers, it's
recommended to set this value to some small percent (e.g. 5%) of the total
query processing cores available to the peon.|0|
Review Comment:
"irrespective of query-level configs." -> did you intend to say query
context like `perSegmentTimeout`? I think it'll be better to be specific here.
##########
docs/querying/query-context-reference.md:
##########
@@ -44,6 +44,7 @@ Unless otherwise noted, the following parameters apply to all
query types, and t
|Parameter |Default | Description
|
|-------------------|----------------------------------------|----------------------|
|`timeout` | `druid.server.http.defaultQueryTimeout`| Query timeout
in millis, beyond which unfinished queries will be cancelled. 0 timeout means
`no timeout` (up to the server-side maximum query timeout,
`druid.server.http.maxQueryTimeout`). To set the default timeout and maximum
timeout, see [Broker configuration](../configuration/index.md#broker) |
+|`perSegmentTimeout`| `null` | Per-segment
processing timeout in millis, beyond which unfinished queries will be
cancelled. Should be ≤ `timeout`. 0 `perSegmentTimeout` means `no per-segment
timeout`. Generally, a standard default should be O(X seconds). A cluster-wide
default value for this query context can be specified via
`druid.query.default.context.perSegmentTimeout`.|
Review Comment:
> Should be ≤ `timeout`
I don't think there's any validation on the query path that checks for this
invariant right? So this is more of a recommendation?
##########
docs/querying/query-context-reference.md:
##########
@@ -44,6 +44,7 @@ Unless otherwise noted, the following parameters apply to all
query types, and t
|Parameter |Default | Description
|
|-------------------|----------------------------------------|----------------------|
|`timeout` | `druid.server.http.defaultQueryTimeout`| Query timeout
in millis, beyond which unfinished queries will be cancelled. 0 timeout means
`no timeout` (up to the server-side maximum query timeout,
`druid.server.http.maxQueryTimeout`). To set the default timeout and maximum
timeout, see [Broker configuration](../configuration/index.md#broker) |
+|`perSegmentTimeout`| `null` | Per-segment
processing timeout in millis, beyond which unfinished queries will be
cancelled. Should be ≤ `timeout`. 0 `perSegmentTimeout` means `no per-segment
timeout`. Generally, a standard default should be O(X seconds). A cluster-wide
default value for this query context can be specified via
`druid.query.default.context.perSegmentTimeout`.|
Review Comment:
```suggestion
|`perSegmentTimeout`| `null` | Per-segment
processing timeout in millis, beyond which unfinished queries occupying the
processing pool will be cancelled. Should be ≤ `timeout`. Setting
`perSegmentTimeout` to 0 means there's no per-segment timeout (up to the
server-side maximum per segment timeout
`druid.query.default.context.perSegmentTimeout`). Generally, a standard default
should be in the order of a few seconds (e.g., 5 seconds). A cluster-wide
default value for this query context can be specified via
`druid.query.default.context.perSegmentTimeout`.|
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
