clintropolis commented on code in PR #20273: URL: https://github.com/apache/druid/pull/20273#discussion_r4029854414
########## server/src/main/java/org/apache/druid/server/metrics/HttpClientPoolMonitor.java: ########## @@ -0,0 +1,85 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.server.metrics; + +import com.google.inject.Inject; +import org.apache.druid.java.util.emitter.service.ServiceEmitter; +import org.apache.druid.java.util.emitter.service.ServiceMetricEvent; +import org.apache.druid.java.util.http.client.pool.ResourcePool; +import org.apache.druid.java.util.metrics.AbstractMonitor; + +import java.util.Map; + +/** + * Emits one row per remote end that an HTTP client of this process pools connections for: what happened since the + * previous emission, plus what that remote end holds right now. Aggregating the rows of a client is left to whoever + * receives them. + * + * This monitor drains the counters of the pools it reads, so it must be the only reader of them. + */ +public class HttpClientPoolMonitor extends AbstractMonitor +{ + private static final String CLIENT_DIMENSION = "httpClient"; + private static final String SERVER_DIMENSION = "server"; + + private final HttpClientPoolRegistry registry; + + @Inject + public HttpClientPoolMonitor(HttpClientPoolRegistry registry) + { + this.registry = registry; + } + + @Override + public boolean doMonitor(ServiceEmitter emitter) + { + for (Map.Entry<String, ResourcePool<?, ?>> client : registry.getPools().entrySet()) { + for (Map.Entry<?, ResourcePool.Stats> pool : client.getValue().drainStats().entrySet()) { + final ResourcePool.Stats stats = pool.getValue(); + final ServiceMetricEvent.Builder builder = ServiceMetricEvent.builder() + .setDimension(CLIENT_DIMENSION, client.getKey()) + .setDimension( + SERVER_DIMENSION, + serverOf(pool.getKey()) + ); + emitter.emit(builder.setMetric("httpClient/pool/opened", stats.opened())); Review Comment: nit: make these metric names public static fields so that the tests can refer to the metric names instead of copying string constants ########## processing/src/main/java/org/apache/druid/java/util/http/client/HttpClientConfig.java: ########## @@ -185,6 +202,8 @@ public static class Builder { private int numConnections = 1; private boolean eagerInitialization = true; + private ResourcePool.Implementation poolImplementation = ResourcePool.Implementation.ADAPTIVE; Review Comment: any justification for why is the new mode the default given that it is a behavior change? ########## docs/configuration/index.md: ########## @@ -1832,8 +1834,11 @@ 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| Review 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 -- 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]
