Repository navigation
feat: add adaptive http pooling - #20273
Conversation
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
Reviewed 8 of 8 changed files.
This is an automated review by Codex GPT-5.6-Luna(max)
| public void close() | ||
| { | ||
| if (closed.compareAndSet(false, true)) { | ||
| permits.release(maxSize); |
There was a problem hiding this comment.
[P2] Wake zero-sized adaptive pools on close
numConnections is validated with @Min(0), so an adaptive pool can legitimately be constructed with maxSize == 0. Its semaphore then starts with zero permits, and close() calls release(maxSize), i.e. release(0), which wakes no thread blocked in acquirePermit(). A request already blocked in ResourcePool.take() therefore remains stuck through client shutdown, whereas the retaining implementation's notifyAll() unblocks it. Release at least one permit for the zero-size case (or use an explicit close signal) so close can always wake waiters.
|
the new configuration should be documented |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| final Injector injector = Initialization.makeInjectorWithModules( | ||
| GuiceInjectors.makeStartupInjector(), | ||
| List.of(testBindings, HttpClientModule.global(), HttpClientModule.escalatedGlobal()) | ||
| ); |
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 2 |
| P3 | 0 |
| Total | 2 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 2 |
| P3 | 0 |
| Total | 2 |
Reviewed 15 of 15 changed files.
This is an automated review by Codex GPT-5.6-Luna(max)
| public class HttpClientInit | ||
| { | ||
| public static HttpClient createClient(HttpClientConfig config, Lifecycle lifecycle) | ||
| public static NettyHttpClient createClient(HttpClientConfig config, Lifecycle lifecycle) |
There was a problem hiding this comment.
[P2] Changing the return type breaks binary callers
This public static method previously returned HttpClient, but the new return type changes the JVM method descriptor to (...):NettyHttpClient. Existing compiled extensions or downstream tests that call HttpClientInit.createClient(...):HttpClient will therefore fail with NoSuchMethodError after upgrading, even though the same source still compiles. Preserve the old signature and add a differently named Netty-returning factory (or expose the pool another way) for the registration code.
| |`druid.router.http.eagerInitialization`|Indicates that http connections from Router to Broker should be eagerly initialized. If set to true, `numConnections` connections are created upon initialization|`true`| | ||
| |`druid.router.http.readTimeout`|The timeout for data reads from Broker processes.|`PT15M`| | ||
| |`druid.router.http.unusedConnectionTimeout`|The timeout for idle connections in connection pool. The connection in the pool will be closed after this timeout and a new one will be established. This timeout should be less than `druid.router.http.readTimeout`. Set this timeout = ~90% of `druid.router.http.readTimeout`|`PT4M`| | ||
| |`druid.router.http.poolImplementation`|How the connection pool follows demand. With `adaptive`, a query discards every stale or broken connection it walks past, so the pool falls back to the size the traffic needs. With `retaining`, the pool keeps every connection it has opened, replacing a stale or broken one by a fresh one, one for one.|`adaptive`| |
There was a problem hiding this comment.
[P2] Router pool settings are not applied
The Router process binds druid.router.http through JettyHttpClientModule, whose provider does not read poolImplementation or strictConnectionValidation; only the Netty HttpClientModule copies these fields into ResourcePoolConfig. Consequently these documented Router settings are silently ignored, so Router does not get the advertised adaptive shrinking or strict validation. Remove these rows or wire the settings into the Router client before documenting them.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟢 Approval recommended
No actionable issues found in this updated-head review. The current head resolves the prior review concerns: HttpClientInit.createClient retains its original HttpClient return descriptor, Router-only pool settings are no longer documented under druid.router.http, and production HTTP client connection counts require at least one connection. The adaptive and retaining pool paths, shutdown wake-up behavior, validation/replacement cleanup, metrics draining, client registration, configuration parsing, and documentation were rechecked against the full current diff and surrounding code.
Reviewed 15 of 15 changed files. The review began with the incremental diff from the previous reviewed head and then covered the full merge-base diff plus relevant call sites and lifecycle/metrics code.
Validation: git diff --check 9b90983fd291f26935af934383ce360473179e4d 5eb69d98ebd9fa9a9de6695f8a7d0f090de0c026 passed.
This is an automated review by Codex GPT-5.6-Luna(max)
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟢 Approval recommended
No actionable issues found in this updated-head review. The incremental patch only updates the HTTP pool monitor test fixtures to use realistic HTTPS pool keys; the full current diff and surrounding code remain consistent with the prior review. The adaptive and retaining pool paths, shutdown wake-up behavior, validation/replacement cleanup, metrics draining, client registration, configuration parsing, API compatibility, and documentation were rechecked, including the prior review concerns.
Reviewed 15 of 15 changed files. The review began with the incremental diff from the previous reviewed head and then covered the full merge-base diff plus relevant call sites and lifecycle/metrics code.
Validation: git diff --check 9b90983fd291f26935af934383ce360473179e4d HEAD passed.
This is an automated review by Codex GPT-5.6-Luna(max)
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟢 Approval recommended
No actionable issues found in this updated-head review. The incremental patch from the previous reviewed head is the merge of apache/master; its overlapping edits are upstream changes rather than new PR changes. I rechecked the full current merge-base diff and surrounding code for adaptive and retaining pool invariants, validation and replacement retries, interruption and shutdown wake-up, resource cleanup, counters and metrics draining, client registration, configuration parsing and validation, public API compatibility, and documentation. The prior packet concerns about the HttpClientInit.createClient return descriptor, router-only documentation, and the minimum connection count remain resolved.
Reviewed 15 of 15 changed files. Exact coverage: docs/configuration/index.md, docs/operations/metrics.md, processing/src/main/java/org/apache/druid/java/util/http/client/HttpClientConfig.java, processing/src/main/java/org/apache/druid/java/util/http/client/HttpClientInit.java, processing/src/main/java/org/apache/druid/java/util/http/client/NettyHttpClient.java, processing/src/main/java/org/apache/druid/java/util/http/client/pool/ResourcePool.java, processing/src/main/java/org/apache/druid/java/util/http/client/pool/ResourcePoolConfig.java, processing/src/test/java/org/apache/druid/java/util/http/client/pool/ResourcePoolTest.java, server/src/main/java/org/apache/druid/guice/http/DruidHttpClientConfig.java, server/src/main/java/org/apache/druid/guice/http/HttpClientModule.java, server/src/main/java/org/apache/druid/server/metrics/HttpClientPoolMonitor.java, server/src/main/java/org/apache/druid/server/metrics/HttpClientPoolRegistry.java, server/src/test/java/org/apache/druid/guice/http/DruidHttpClientConfigTest.java, server/src/test/java/org/apache/druid/guice/http/HttpClientPoolRegistrationTest.java, and server/src/test/java/org/apache/druid/server/metrics/HttpClientPoolMonitorTest.java.
Validation: git diff --check e3f06eac3ec902937f72caa31871cf97d5a1df12 f79cd8f60265e3b5ff8f76f3c7139cdf8e445280 passed.
This is an automated review by Codex GPT-5.6-Luna(max)
| |`druid.broker.http.unusedConnectionTimeout`|The timeout for idle connections in connection pool. The connection in the pool will be closed after this timeout and a new one will be established. This timeout should be less than `druid.broker.http.readTimeout`. Set this timeout = ~90% of `druid.broker.http.readTimeout`|`PT4M`| | ||
| |`druid.broker.http.maxQueuedBytes`|Maximum number of bytes queued per query before exerting [backpressure](../operations/basic-cluster-tuning.md#broker-backpressure) on channels to the data servers.<br /><br />Similar to `druid.server.http.maxScatterGatherBytes`, except that `maxQueuedBytes` triggers [backpressure](../operations/basic-cluster-tuning.md#broker-backpressure) instead of query failure. Set to zero to disable. You can override this setting by using the [`maxQueuedBytes` query context parameter](../querying/query-context-reference.md). Druid supports [human-readable](human-readable-byte.md) format. |25 MB or 2% of maximum Broker heap size, whichever is greater.| | ||
| |`druid.broker.http.numMaxThreads`|`Maximum number of I/O worker threads|(number of cores) * 3 / 2 + 1`| | ||
| |`druid.broker.http.clientConnectTimeout`|The timeout (in milliseconds) for establishing client connections.|500| |
There was a problem hiding this comment.
this config only applies to request forwarding that uses the jetty http client, my conclusion in #19567 was that this didn't apply to the broker so i don't think we need to mention this config here because it isn't wired up to anything so i removed it there
There was a problem hiding this comment.
this appeared in a merge against master in my changes...
I think I must have accepted the line which was rewritten to be the connectTimeout
removed it
| SERVER_DIMENSION, | ||
| serverOf(pool.getKey()) | ||
| ); | ||
| emitter.emit(builder.setMetric("httpClient/pool/opened", stats.opened())); |
There was a problem hiding this comment.
nit: make these metric names public static fields so that the tests can refer to the metric names instead of copying string constants
There was a problem hiding this comment.
changed it; what do you think about these metrics overall?
probably taken + used will be the most useful ones of them - as those will help understand the actual usage a lot...all the others are less interesting....
but since this is pluggable I went on a rampage - people who don't need it will not load it :D
| { | ||
| private int numConnections = 1; | ||
| private boolean eagerInitialization = true; | ||
| private ResourcePool.Implementation poolImplementation = ResourcePool.Implementation.ADAPTIVE; |
There was a problem hiding this comment.
any justification for why is the new mode the default given that it is a behavior change?
There was a problem hiding this comment.
I think the old approach is very bad....as it can't scale down at all; which could leading to 1:1 replacement of bad connections every time a request hit those pools...which could happen if load is uneven on servers
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟢 Approval recommended
No actionable issues found in this updated-head review. The incremental patch from the previous reviewed head is the upstream master merge plus the final HTTP pool metric-name constant refactor and test fixture update; I rechecked the full current merge-base diff and surrounding code. The prior concerns about the HttpClientInit.createClient return descriptor, router-only configuration documentation, and the minimum connection count remain resolved.
Reviewed 15 of 15 changed files. Exact coverage: docs/configuration/index.md, docs/operations/metrics.md, processing/src/main/java/org/apache/druid/java/util/http/client/HttpClientConfig.java, processing/src/main/java/org/apache/druid/java/util/http/client/HttpClientInit.java, processing/src/main/java/org/apache/druid/java/util/http/client/NettyHttpClient.java, processing/src/main/java/org/apache/druid/java/util/http/client/pool/ResourcePool.java, processing/src/main/java/org/apache/druid/java/util/http/client/pool/ResourcePoolConfig.java, processing/src/test/java/org/apache/druid/java/util/http/client/pool/ResourcePoolTest.java, server/src/main/java/org/apache/druid/guice/http/DruidHttpClientConfig.java, server/src/main/java/org/apache/druid/guice/http/HttpClientModule.java, server/src/main/java/org/apache/druid/server/metrics/HttpClientPoolMonitor.java, server/src/main/java/org/apache/druid/server/metrics/HttpClientPoolRegistry.java, server/src/test/java/org/apache/druid/guice/http/DruidHttpClientConfigTest.java, server/src/test/java/org/apache/druid/guice/http/HttpClientPoolRegistrationTest.java, and server/src/test/java/org/apache/druid/server/metrics/HttpClientPoolMonitorTest.java.
Validation: git diff --check 2fed7ca11d9852412ce76798a3fa150ddae9afdb 234c00aae78e3bb2ce73c75a7d701a3d777b6750 and git diff --check f79cd8f60265e3b5ff8f76f3c7139cdf8e445280 234c00aae78e3bb2ce73c75a7d701a3d777b6750 passed.
This is an automated review by Codex GPT-5.6-Luna(max)
| |`httpClient/pool/timedOut`|Number of connections discarded for being unused longer than `unusedConnectionTimeout`.|`httpClient`, `server`|Varies| | ||
| |`httpClient/pool/taken`|Number of connections handed to a caller, that is, the number of requests that got a connection.|`httpClient`, `server`|Varies| | ||
| |`httpClient/pool/returned`|Number of connections given back by a caller.|`httpClient`, `server`|Close to `httpClient/pool/taken`| | ||
| |`httpClient/pool/used`|Number of connections in the hands of callers at the time of the emission, that is, the requests in flight to that remote end. A level, not a per period count.|`httpClient`, `server`|<= `druid.<service>.http.numConnections`. Sitting at that ceiling means requests are waiting for a connection.| |
There was a problem hiding this comment.
ci website build doesn't seem to like the "<=" in the last column https://github.com/apache/druid/actions/runs/35212764228/job/105173968390?pr=20273#step:7:104
presumably the line that follows would also have the same problem since it has the same pattern
There was a problem hiding this comment.
oh - sorry ; I was thinking that "I did not made any changes to the site" ...and it must be unrelated 🤦
fixed it
|
|
||
| private static class AdaptiveResourceHolderPerKey<K, V> extends PooledResources<V> | ||
| { | ||
| private static final int CREATE_ATTEMPTS = 3; |
There was a problem hiding this comment.
what do you think about making this configurable? I'm sort of worried about this amplifying problems a sad historical might be having by retrying connections in exchange for being to handle unlikely blips.
I think we could consider removing the old strategies if this was configurable since overall this mode seems better in every way and the only major difference is this retrying, so if it could be turned off then I think there is low value in retaining the old strategies. That said, I think it would be fine to leave the old strategies for a release or 2 too just in case.
There was a problem hiding this comment.
in the end the whole attempts stuff was removed; the old logic was:
- with
eager=false- when there was none avail in the pool: a connection was created
- then later it was checked with
isGood- if it failed it was created again
- with
eager=true- the pool was filled up - so it could not happened that it was not in the pool - unless it was at limit
- it was recreated when it was bad...
since eager=true was set by default ; the whole attempt stuff should be off the table - everything was working fine with that...and should work.
The only thing I kept from this detour is a log message which might help to know if we are encountering any issues.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟢 Approval recommended
No actionable issues found in this updated-head review. The incremental patch removes the obsolete strict-validation option and updates its pool documentation/tests; the full current diff remains consistent with the adaptive and retaining pool invariants, validation and cleanup paths, shutdown wake-up behavior, metrics draining, client registration, configuration parsing, API compatibility, and documented scope.
Reviewed 15 of 15 changed files. Exact coverage: docs/configuration/index.md, docs/operations/metrics.md, processing/src/main/java/org/apache/druid/java/util/http/client/HttpClientConfig.java, processing/src/main/java/org/apache/druid/java/util/http/client/HttpClientInit.java, processing/src/main/java/org/apache/druid/java/util/http/client/NettyHttpClient.java, processing/src/main/java/org/apache/druid/java/util/http/client/pool/ResourcePool.java, processing/src/main/java/org/apache/druid/java/util/http/client/pool/ResourcePoolConfig.java, processing/src/test/java/org/apache/druid/java/util/http/client/pool/ResourcePoolTest.java, server/src/main/java/org/apache/druid/guice/http/DruidHttpClientConfig.java, server/src/main/java/org/apache/druid/guice/http/HttpClientModule.java, server/src/main/java/org/apache/druid/server/metrics/HttpClientPoolMonitor.java, server/src/main/java/org/apache/druid/server/metrics/HttpClientPoolRegistry.java, server/src/test/java/org/apache/druid/guice/http/DruidHttpClientConfigTest.java, server/src/test/java/org/apache/druid/guice/http/HttpClientPoolRegistrationTest.java, and server/src/test/java/org/apache/druid/server/metrics/HttpClientPoolMonitorTest.java.
The review began with the incremental diff from the previous reviewed head, then covered the full current merge-base diff and relevant callers/contracts. Validation: git diff --check 2fed7ca e12e262 passed.
This is an automated review by Codex GPT-5.6-Luna(max)
|
thank you @clintropolis for reviewing the changes! |
4 participants
The ResourcePool class employed a non-shrinking pooling mechanism; with eager enabled that have bumped up the connections to the max.
If the query rate was low - or some nodes were not really participating in some queries; their pool could have become cold - and because of the 1:1 replacement policy it could have never scaled down.. this PR adds an adaptive pool implementation and defaults to it. The pool will still be pumped up to maxConnections if eager is enabled; however it will scale back to match usage pattern better.