kgyrtkirk commented on code in PR #20273: URL: https://github.com/apache/druid/pull/20273#discussion_r4035780475
########## 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: 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 ########## 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: 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 ########## 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 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 -- 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]
