gianm commented on code in PR #19567:
URL: https://github.com/apache/druid/pull/19567#discussion_r3982547400
##########
processing/src/main/java/org/apache/druid/java/util/http/client/HttpClientInit.java:
##########
@@ -125,37 +93,24 @@ public static SSLContext
sslContextWithTrustedKeyStore(final String keyStorePath
}
}
- private static ClientBootstrap createBootstrap(Lifecycle lifecycle, Timer
timer, int bossPoolSize, int workerPoolSize)
+ private static Bootstrap createBootstrap(Lifecycle lifecycle, int
workerPoolSize, Duration connectTimeout)
Review Comment:
Let's explicitly configure an allocator here, just so we know what we're
getting.
##########
processing/src/main/java/org/apache/druid/java/util/http/client/NettyHttpClient.java:
##########
@@ -125,49 +118,43 @@ public <Intermediate, Final> ListenableFuture<Final> go(
final Channel channel;
final String hostKey = getPoolKey(url);
final ResourceContainer<ChannelFuture> channelResourceContainer =
pool.take(hostKey);
- final ChannelFuture channelFuture =
channelResourceContainer.get().awaitUninterruptibly();
+ // pool.take() returns null only when the pool is closed (the HttpClient
has been stopped); that path
+ // already logs at ERROR inside ResourcePool. Surface it here so the
caller sees a real exception
+ // instead of an NPE.
+ if (channelResourceContainer == null) {
+ return Futures.immediateFailedFuture(
+ new ChannelException(
+ StringUtils.format("HttpClient is closed; cannot obtain a
channel for host[%s]", hostKey)
+ )
+ );
+ }
+ // channelResourceContainer.get() itself can be null when the underlying
holder was interrupted (or
+ // the pool was closed) while waiting for capacity; in either case the
pool never incremented its
+ // lent-resources count, so we do NOT call returnResource() (which would
NPE inside giveBack); we
+ // just fail the request.
+ final ChannelFuture rawChannelFuture = channelResourceContainer.get();
+ if (rawChannelFuture == null) {
+ return Futures.immediateFailedFuture(
+ new ChannelException(
+ StringUtils.format("Interrupted or pool closed while waiting for
a channel to host[%s]", hostKey)
+ )
+ );
+ }
+ final ChannelFuture channelFuture =
rawChannelFuture.awaitUninterruptibly();
Review Comment:
If this fails, or if `channel.config().setAutoRead(true)` below fails, then
the resource is leaked. I'm not sure if this can happen but it would be nice to
guard against it.
##########
docs/configuration/index.md:
##########
@@ -2331,4 +2333,5 @@ Supported query contexts:
|`druid.router.http.numMaxThreads`|Maximum number of worker threads to handle
HTTP requests and responses|`(number of cores) * 3 / 2 + 1`|
|`druid.router.http.numRequestsQueued`|Maximum number of requests that may be
queued to a destination|`1024`|
|`druid.router.http.requestBuffersize`|Size of the content buffer for
receiving requests. These buffers are only used for active connections that
have requests with bodies that will not fit within the header buffer|`8 * 1024`|
-|`druid.router.http.clientConnectTimeout`|The timeout (in milliseconds) for
establishing client connections.|500|
+|`druid.router.http.clientConnectTimeout`|Connect timeout (in milliseconds)
for the HTTP client the Router uses to forward incoming queries and management
requests to Brokers and other Druid services. Does not affect the Router's
direct RPC connections; see `connectTimeout` for those.|500|
+|`druid.router.http.connectTimeout`|Connect timeout for the HTTP client the
Router uses for direct RPC to Brokers (for example, service-status
polling).|`PT10S`|
Review Comment:
I believe this config may be fake (because we don't use the NettyHttpClient
for Router -> Broker communications). Please double check it.
##########
server/src/main/java/org/apache/druid/client/DirectDruidClient.java:
##########
@@ -423,11 +425,16 @@ public ClientResponse<InputStream>
done(ClientResponse<InputStream> clientRespon
@Override
public void exceptionCaught(final ClientResponse<InputStream>
clientResponse, final Throwable e)
{
+ // Fall back to Throwable.toString() when the exception carries no
message, so a timeout
+ // (Netty's ReadTimeoutException is a stackless, messageless
singleton) does not render as
+ // "exception msg [null]" but as "exception msg
[io.netty.handler.timeout.ReadTimeoutException]"
+ // instead. Behaviour for exceptions that DO have a message is
unchanged.
+ final String exceptionDetail = e.getMessage() != null ?
e.getMessage() : e.toString();
Review Comment:
Please do a similar thing in `DataServerResponseHandler`.
##########
docs/configuration/index.md:
##########
@@ -1813,7 +1814,8 @@ client has the following configuration options.
|`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|
+|`druid.broker.http.clientConnectTimeout`|Connect timeout (in milliseconds)
for the Broker's request-forwarding HTTP client. Does not affect the Broker's
direct connections to data servers; see `connectTimeout` for those.|500|
Review Comment:
I don't think the Broker has a request-forwarding HTTP client? Please double
check this.
##########
docs/configuration/index.md:
##########
@@ -678,7 +678,8 @@ All Druid components can communicate with each other over
HTTP.
|`druid.global.http.readTimeout`|The timeout for data reads.|`PT15M`|
|`druid.global.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.global.http.readTimeout`. Set this timeout = ~90% of
`druid.global.http.readTimeout`|`PT4M`|
|`druid.global.http.numMaxThreads`|Maximum number of I/O worker
threads|`(number of cores) * 3 / 2 + 1`|
-|`druid.global.http.clientConnectTimeout`|The timeout (in milliseconds) for
establishing client connections.|500|
+|`druid.global.http.clientConnectTimeout`|Connect timeout (in milliseconds)
for the HTTP client used to forward requests between Druid services (for
example, when the Router proxies queries to Brokers, or when management API
calls are forwarded to the Coordinator or Overlord). Does not affect direct RPC
connections between services; see `connectTimeout` for those.|500|
Review Comment:
I believe this overstates what this is used for. Isn't the
`druid.global.http` client used for other stuff beyond forwarding requests?
Like, I thought it was also used for most internal RPCs generally via the
`@EscalatedGlobal` client.
--
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]
