You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "ankitsultana (via GitHub)" <gi...@apache.org> on 2023/03/27 23:51:55 UTC
[GitHub] [pinot] ankitsultana opened a new pull request, #10487: Fix Bottleneck for Server Bootstrap by Making maxConnsPerRoute Configurable
ankitsultana opened a new pull request, #10487:
URL: https://github.com/apache/pinot/pull/10487
At present we don't set any configs when we create the HttpClient in `FileUploadDownloadClient`. By default, Apache Commons HttpClient uses maxTotalConns=20 and maxConnsPerRoute=2 [ref](https://www.javadoc.io/static/org.apache.httpcomponents/httpclient/4.5.3/org/apache/http/impl/conn/PoolingClientConnectionManager.html#:~:text=PoolingConnectionManager%20maintains%20a%20maximum%20limit,more%2020%20connections%20in%20total.).
This can become a bottleneck when we need to replace a server, since in that case the new server may have to download 1000s of segments. You can find a sample trace from a thread-dump we took in our clusters below.
This patch makes this configurable. Since apache http-client is used in a bunch of places, each corresponding use-case can add their own config prefix.
The suffix of the config is enforced by `HttpClientConfig.newBuilder(PinotConfiguration, String)` in an attempt to ensure standarization across the use-cases.
```
"HelixTaskExecutor-message_handle_thread" #226 daemon prio=5 os_prio=0 cpu=122724.33ms elapsed=4094.44s tid=0x00007f41cc007800 nid=0x140 waiting on condition [0x00007f3f969ea000]
--
java.lang.Thread.State: WAITING (parking)
at jdk.internal.misc.Unsafe.park(java.base@11.0.15/Native Method)
- parking to wait for <0x00007f4595c10118> (a java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject)
at java.util.concurrent.locks.LockSupport.park(java.base@11.0.15/LockSupport.java:194)
at java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject.await(java.base@11.0.15/AbstractQueuedSynchronizer.java:2081)
at org.apache.http.pool.AbstractConnPool.getPoolEntryBlocking(AbstractConnPool.java:393)
at org.apache.http.pool.AbstractConnPool.access$300(AbstractConnPool.java:70)
at org.apache.http.pool.AbstractConnPool$2.get(AbstractConnPool.java:253)
- locked <0x00007f4f3edda4a8> (a org.apache.http.pool.AbstractConnPool$2)
at org.apache.http.pool.AbstractConnPool$2.get(AbstractConnPool.java:198)
at org.apache.http.impl.conn.PoolingHttpClientConnectionManager.leaseConnection(PoolingHttpClientConnectionManager.java:306)
at org.apache.http.impl.conn.PoolingHttpClientConnectionManager$1.get(PoolingHttpClientConnectionManager.java:282)
at org.apache.http.impl.execchain.MainClientExec.execute(MainClientExec.java:190)
at org.apache.http.impl.execchain.ProtocolExec.execute(ProtocolExec.java:186)
at org.apache.http.impl.execchain.RetryExec.execute(RetryExec.java:89)
at org.apache.http.impl.execchain.RedirectExec.execute(RedirectExec.java:110)
at org.apache.http.impl.client.InternalHttpClient.doExecute(InternalHttpClient.java:185)
at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:83)
at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:108)
at org.apache.pinot.common.utils.http.HttpClient.downloadFile(HttpClient.java:334)
at org.apache.pinot.common.utils.FileUploadDownloadClient.downloadFile(FileUploadDownloadClient.java:1005)
at org.apache.pinot.common.utils.fetcher.HttpSegmentFetcher.lambda$fetchSegmentToLocal$0(HttpSegmentFetcher.java:66)
at org.apache.pinot.common.utils.fetcher.HttpSegmentFetcher$Lambda$438/0x00007f3ebd9ed908.call(Unknown Source)
at org.apache.pinot.spi.utils.retry.BaseRetryPolicy.attempt(BaseRetryPolicy.java:50)
at org.apache.pinot.common.utils.fetcher.HttpSegmentFetcher.fetchSegmentToLocal(HttpSegmentFetcher.java:54)
at org.apache.pinot.common.utils.fetcher.SegmentFetcherFactory.fetchSegmentToLocalInternal(SegmentFetcherFactory.java:152)
at org.apache.pinot.common.utils.fetcher.SegmentFetcherFactory.fetchSegmentToLocal(SegmentFetcherFactory.java:138)
at org.apache.pinot.common.utils.fetcher.SegmentFetcherFactory.fetchAndDecryptSegmentToLocalInternal(SegmentFetcherFactory.java:191)
at org.apache.pinot.common.utils.fetcher.SegmentFetcherFactory.fetchAndDecryptSegmentToLocal(SegmentFetcherFactory.java:168)
at org.apache.pinot.core.data.manager.BaseTableDataManager.downloadFromPeersWithoutStreaming(BaseTableDataManager.java:512)
at org.apache.pinot.core.data.manager.BaseTableDataManager.downloadAndDecrypt(BaseTableDataManager.java:484)
at org.apache.pinot.core.data.manager.BaseTableDataManager.downloadSegmentFromDeepStore(BaseTableDataManager.java:450)
at org.apache.pinot.core.data.manager.BaseTableDataManager.downloadSegment(BaseTableDataManager.java:442)
at org.apache.pinot.core.data.manager.BaseTableDataManager.addOrReplaceSegment(BaseTableDataManager.java:404)
at org.apache.pinot.server.starter.helix.HelixInstanceDataManager.addOrReplaceSegment(HelixInstanceDataManager.java:355)
at org.apache.pinot.server.starter.helix.SegmentOnlineOfflineStateModelFactory$SegmentOnlineOfflineStateModel.onBecomeOnlineFromOffline(SegmentOnlineOfflineStateModelFactory.java:162)
```
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ankitsultana commented on a diff in pull request #10487: Fix Bottleneck for Server Bootstrap by Making maxConnsPerRoute Configurable
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #10487:
URL: https://github.com/apache/pinot/pull/10487#discussion_r1155166118
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/http/HttpClient.java:
##########
@@ -86,22 +87,23 @@ public class HttpClient implements AutoCloseable {
private final CloseableHttpClient _httpClient;
public HttpClient() {
- this(null);
+ this(HttpClientConfig.DEFAULT_HTTP_CLIENT_CONFIG, null);
}
- public HttpClient(@Nullable SSLContext sslContext) {
+ public HttpClient(HttpClientConfig httpClientConfig, @Nullable SSLContext sslContext) {
Review Comment:
How so? Is anyone extending this Class and using their own HttpClient implementations?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10487: Fix Bottleneck for Server Bootstrap by Making maxConnsPerRoute Configurable
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10487:
URL: https://github.com/apache/pinot/pull/10487#discussion_r1155085289
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/http/HttpClient.java:
##########
@@ -86,22 +87,23 @@ public class HttpClient implements AutoCloseable {
private final CloseableHttpClient _httpClient;
public HttpClient() {
- this(null);
+ this(HttpClientConfig.DEFAULT_HTTP_CLIENT_CONFIG, null);
}
- public HttpClient(@Nullable SSLContext sslContext) {
+ public HttpClient(HttpClientConfig httpClientConfig, @Nullable SSLContext sslContext) {
Review Comment:
This is a change that breaks backward compatibility. We should add another constructor with only `SSLContext` as parameter
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10487: Fix Bottleneck for Server Bootstrap by Making maxConnsPerRoute Configurable
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10487:
URL: https://github.com/apache/pinot/pull/10487#discussion_r1152553543
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/http/HttpClient.java:
##########
@@ -86,22 +86,24 @@ public class HttpClient implements AutoCloseable {
private final CloseableHttpClient _httpClient;
public HttpClient() {
- this(null);
+ this(HttpClientConfig.DEFAULT_HTTP_CLIENT_CONFIG, null);
}
- public HttpClient(@Nullable SSLContext sslContext) {
+ public HttpClient(HttpClientConfig httpClientConfig, @Nullable SSLContext sslContext) {
SSLContext context = sslContext != null ? sslContext : TlsUtils.getSslContext();
// Set NoopHostnameVerifier to skip validating hostname when uploading/downloading segments.
SSLConnectionSocketFactory csf = new SSLConnectionSocketFactory(context, NoopHostnameVerifier.INSTANCE);
- _httpClient = HttpClients.custom().setSSLSocketFactory(csf).build();
+ _httpClient = HttpClients.custom().setMaxConnTotal(httpClientConfig.getMaxConns())
Review Comment:
To keep the existing behavior, we can set maxConns or maxConnsPerRoute only when the value from the config > 0, and make 0 the default value if config does not exist
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/http/HttpClientConfig.java:
##########
@@ -0,0 +1,92 @@
+/**
+ * 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.pinot.common.utils.http;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+public class HttpClientConfig {
+ // Default config uses default values which are same as what Apache Commons Http-Client uses.
+ public static final HttpClientConfig DEFAULT_HTTP_CLIENT_CONFIG = HttpClientConfig.newBuilder().build();
+
+ protected static final String MAX_CONNS_CONFIG_NAME = "http.client.max_conns";
+ protected static final String MAX_CONNS_PER_ROUTE_CONFIG_NAME = "http.client.max_conns_per_route";
Review Comment:
We use camel case for config keys. Also should we consider using the same name as the internal key for HttpClient (`maxConnTotal` and `maxConnPerRoute`)?
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/http/HttpClientConfig.java:
##########
@@ -0,0 +1,92 @@
+/**
+ * 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.pinot.common.utils.http;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+public class HttpClientConfig {
+ // Default config uses default values which are same as what Apache Commons Http-Client uses.
+ public static final HttpClientConfig DEFAULT_HTTP_CLIENT_CONFIG = HttpClientConfig.newBuilder().build();
+
+ protected static final String MAX_CONNS_CONFIG_NAME = "http.client.max_conns";
+ protected static final String MAX_CONNS_PER_ROUTE_CONFIG_NAME = "http.client.max_conns_per_route";
+
+ private final int _maxConns;
+ private final int _maxConnsPerRoute;
+
+ private HttpClientConfig(int maxConns, int maxConnsPerRoute) {
+ _maxConns = maxConns;
+ _maxConnsPerRoute = maxConnsPerRoute;
+ }
+
+ public int getMaxConns() {
+ return _maxConns;
+ }
+
+ public int getMaxConnsPerRoute() {
+ return _maxConnsPerRoute;
+ }
+
+ /**
+ * Creates a {@link HttpClientConfig.Builder} and initializes it with relevant configs from the provided
+ * configuration. Since http-clients are used in a bunch of places in the code, each use-case can have their own
+ * prefix for their config. The caller should call {@link PinotConfiguration#subset(String)} to remove their prefix
+ * and this builder will look for exact matches of its relevant configs.
+ */
+ public static Builder newBuilder(PinotConfiguration pinotConfiguration) {
+ Builder builder = new Builder();
+ String maxConns = pinotConfiguration.getProperty(MAX_CONNS_CONFIG_NAME);
+ if (StringUtils.isNotEmpty(maxConns)) {
+ builder.withMaxConns(Integer.parseInt(maxConns));
+ }
+ String maxConnsPerRoute = pinotConfiguration.getProperty(MAX_CONNS_PER_ROUTE_CONFIG_NAME);
+ if (StringUtils.isNotEmpty(maxConnsPerRoute)) {
+ builder.withMaxConnsPerRoute(Integer.parseInt(maxConnsPerRoute));
+ }
+ return builder;
+ }
+
+ private static Builder newBuilder() {
+ return new Builder();
+ }
+
+ public static class Builder {
+ private int _maxConns = 20;
+ private int _maxConnsPerRoute = 2;
Review Comment:
Where are these 2 default values come from? We should try to avoid magic number
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ankitsultana commented on a diff in pull request #10487: Fix Bottleneck for Server Bootstrap by Making maxConnsPerRoute Configurable
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #10487:
URL: https://github.com/apache/pinot/pull/10487#discussion_r1153692836
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/http/HttpClientConfig.java:
##########
@@ -0,0 +1,92 @@
+/**
+ * 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.pinot.common.utils.http;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+public class HttpClientConfig {
+ // Default config uses default values which are same as what Apache Commons Http-Client uses.
+ public static final HttpClientConfig DEFAULT_HTTP_CLIENT_CONFIG = HttpClientConfig.newBuilder().build();
+
+ protected static final String MAX_CONNS_CONFIG_NAME = "http.client.max_conns";
+ protected static final String MAX_CONNS_PER_ROUTE_CONFIG_NAME = "http.client.max_conns_per_route";
+
+ private final int _maxConns;
+ private final int _maxConnsPerRoute;
+
+ private HttpClientConfig(int maxConns, int maxConnsPerRoute) {
+ _maxConns = maxConns;
+ _maxConnsPerRoute = maxConnsPerRoute;
+ }
+
+ public int getMaxConns() {
+ return _maxConns;
+ }
+
+ public int getMaxConnsPerRoute() {
+ return _maxConnsPerRoute;
+ }
+
+ /**
+ * Creates a {@link HttpClientConfig.Builder} and initializes it with relevant configs from the provided
+ * configuration. Since http-clients are used in a bunch of places in the code, each use-case can have their own
+ * prefix for their config. The caller should call {@link PinotConfiguration#subset(String)} to remove their prefix
+ * and this builder will look for exact matches of its relevant configs.
+ */
+ public static Builder newBuilder(PinotConfiguration pinotConfiguration) {
+ Builder builder = new Builder();
+ String maxConns = pinotConfiguration.getProperty(MAX_CONNS_CONFIG_NAME);
+ if (StringUtils.isNotEmpty(maxConns)) {
+ builder.withMaxConns(Integer.parseInt(maxConns));
+ }
+ String maxConnsPerRoute = pinotConfiguration.getProperty(MAX_CONNS_PER_ROUTE_CONFIG_NAME);
+ if (StringUtils.isNotEmpty(maxConnsPerRoute)) {
+ builder.withMaxConnsPerRoute(Integer.parseInt(maxConnsPerRoute));
+ }
+ return builder;
+ }
+
+ private static Builder newBuilder() {
+ return new Builder();
+ }
+
+ public static class Builder {
+ private int _maxConns = 20;
+ private int _maxConnsPerRoute = 2;
Review Comment:
Done
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang merged pull request #10487: Fix Bottleneck for Server Bootstrap by Making maxConnsPerRoute Configurable
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10487:
URL: https://github.com/apache/pinot/pull/10487
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ankitsultana commented on a diff in pull request #10487: Fix Bottleneck for Server Bootstrap by Making maxConnsPerRoute Configurable
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #10487:
URL: https://github.com/apache/pinot/pull/10487#discussion_r1152564734
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/http/HttpClientConfig.java:
##########
@@ -0,0 +1,92 @@
+/**
+ * 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.pinot.common.utils.http;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+public class HttpClientConfig {
+ // Default config uses default values which are same as what Apache Commons Http-Client uses.
+ public static final HttpClientConfig DEFAULT_HTTP_CLIENT_CONFIG = HttpClientConfig.newBuilder().build();
+
+ protected static final String MAX_CONNS_CONFIG_NAME = "http.client.max_conns";
+ protected static final String MAX_CONNS_PER_ROUTE_CONFIG_NAME = "http.client.max_conns_per_route";
Review Comment:
Sounds good. Will update.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ankitsultana commented on a diff in pull request #10487: Fix Bottleneck for Server Bootstrap by Making maxConnsPerRoute Configurable
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #10487:
URL: https://github.com/apache/pinot/pull/10487#discussion_r1152566534
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/http/HttpClientConfig.java:
##########
@@ -0,0 +1,92 @@
+/**
+ * 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.pinot.common.utils.http;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+public class HttpClientConfig {
+ // Default config uses default values which are same as what Apache Commons Http-Client uses.
+ public static final HttpClientConfig DEFAULT_HTTP_CLIENT_CONFIG = HttpClientConfig.newBuilder().build();
+
+ protected static final String MAX_CONNS_CONFIG_NAME = "http.client.max_conns";
+ protected static final String MAX_CONNS_PER_ROUTE_CONFIG_NAME = "http.client.max_conns_per_route";
+
+ private final int _maxConns;
+ private final int _maxConnsPerRoute;
+
+ private HttpClientConfig(int maxConns, int maxConnsPerRoute) {
+ _maxConns = maxConns;
+ _maxConnsPerRoute = maxConnsPerRoute;
+ }
+
+ public int getMaxConns() {
+ return _maxConns;
+ }
+
+ public int getMaxConnsPerRoute() {
+ return _maxConnsPerRoute;
+ }
+
+ /**
+ * Creates a {@link HttpClientConfig.Builder} and initializes it with relevant configs from the provided
+ * configuration. Since http-clients are used in a bunch of places in the code, each use-case can have their own
+ * prefix for their config. The caller should call {@link PinotConfiguration#subset(String)} to remove their prefix
+ * and this builder will look for exact matches of its relevant configs.
+ */
+ public static Builder newBuilder(PinotConfiguration pinotConfiguration) {
+ Builder builder = new Builder();
+ String maxConns = pinotConfiguration.getProperty(MAX_CONNS_CONFIG_NAME);
+ if (StringUtils.isNotEmpty(maxConns)) {
+ builder.withMaxConns(Integer.parseInt(maxConns));
+ }
+ String maxConnsPerRoute = pinotConfiguration.getProperty(MAX_CONNS_PER_ROUTE_CONFIG_NAME);
+ if (StringUtils.isNotEmpty(maxConnsPerRoute)) {
+ builder.withMaxConnsPerRoute(Integer.parseInt(maxConnsPerRoute));
+ }
+ return builder;
+ }
+
+ private static Builder newBuilder() {
+ return new Builder();
+ }
+
+ public static class Builder {
+ private int _maxConns = 20;
+ private int _maxConnsPerRoute = 2;
Review Comment:
These are the current default values: https://www.javadoc.io/static/org.apache.httpcomponents/httpclient/4.5.3/org/apache/http/impl/conn/PoolingClientConnectionManager.html#:~:text=PoolingConnectionManager%20maintains%20a%20maximum%20limit,more%2020%20connections%20in%20total.
Perhaps I can add a comment here and refer to this?
I am also fine with setting these to -1 or something, and in `HttpClient` we can set these only if they are positive. Lmk your thoughts.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on pull request #10487: Fix Bottleneck for Server Bootstrap by Making maxConnsPerRoute Configurable
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #10487:
URL: https://github.com/apache/pinot/pull/10487#issuecomment-1491053216
Can you help update the pinot documentation?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #10487: Fix Bottleneck for Server Bootstrap by Making maxConnsPerRoute Configurable
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10487:
URL: https://github.com/apache/pinot/pull/10487#issuecomment-1486045217
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10487?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10487](https://codecov.io/gh/apache/pinot/pull/10487?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (281cd22) into [master](https://codecov.io/gh/apache/pinot/commit/316842f66e8470ba03042bd5e4d41e09e1d73492?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (316842f) will **decrease** coverage by `56.33%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10487 +/- ##
=============================================
- Coverage 70.30% 13.97% -56.33%
+ Complexity 6113 237 -5876
=============================================
Files 2070 2017 -53
Lines 112037 109552 -2485
Branches 17071 16765 -306
=============================================
- Hits 78765 15315 -63450
- Misses 27721 92984 +65263
+ Partials 5551 1253 -4298
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.97% <0.00%> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10487?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...e/pinot/common/utils/FileUploadDownloadClient.java](https://codecov.io/gh/apache/pinot/pull/10487?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvRmlsZVVwbG9hZERvd25sb2FkQ2xpZW50LmphdmE=) | `0.00% <0.00%> (-57.57%)` | :arrow_down: |
| [...pinot/common/utils/fetcher/HttpSegmentFetcher.java](https://codecov.io/gh/apache/pinot/pull/10487?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZmV0Y2hlci9IdHRwU2VnbWVudEZldGNoZXIuamF2YQ==) | `0.00% <0.00%> (-35.30%)` | :arrow_down: |
| [...inot/common/utils/fetcher/HttpsSegmentFetcher.java](https://codecov.io/gh/apache/pinot/pull/10487?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZmV0Y2hlci9IdHRwc1NlZ21lbnRGZXRjaGVyLmphdmE=) | `0.00% <0.00%> (-58.34%)` | :arrow_down: |
| [...org/apache/pinot/common/utils/http/HttpClient.java](https://codecov.io/gh/apache/pinot/pull/10487?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaHR0cC9IdHRwQ2xpZW50LmphdmE=) | `0.00% <0.00%> (-75.28%)` | :arrow_down: |
| [...ache/pinot/common/utils/http/HttpClientConfig.java](https://codecov.io/gh/apache/pinot/pull/10487?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaHR0cC9IdHRwQ2xpZW50Q29uZmlnLmphdmE=) | `0.00% <0.00%> (ø)` | |
... and [1608 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10487/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10487: Fix Bottleneck for Server Bootstrap by Making maxConnsPerRoute Configurable
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10487:
URL: https://github.com/apache/pinot/pull/10487#discussion_r1153648557
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/http/HttpClientConfig.java:
##########
@@ -0,0 +1,92 @@
+/**
+ * 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.pinot.common.utils.http;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+public class HttpClientConfig {
+ // Default config uses default values which are same as what Apache Commons Http-Client uses.
+ public static final HttpClientConfig DEFAULT_HTTP_CLIENT_CONFIG = HttpClientConfig.newBuilder().build();
+
+ protected static final String MAX_CONNS_CONFIG_NAME = "http.client.max_conns";
+ protected static final String MAX_CONNS_PER_ROUTE_CONFIG_NAME = "http.client.max_conns_per_route";
+
+ private final int _maxConns;
+ private final int _maxConnsPerRoute;
+
+ private HttpClientConfig(int maxConns, int maxConnsPerRoute) {
+ _maxConns = maxConns;
+ _maxConnsPerRoute = maxConnsPerRoute;
+ }
+
+ public int getMaxConns() {
+ return _maxConns;
+ }
+
+ public int getMaxConnsPerRoute() {
+ return _maxConnsPerRoute;
+ }
+
+ /**
+ * Creates a {@link HttpClientConfig.Builder} and initializes it with relevant configs from the provided
+ * configuration. Since http-clients are used in a bunch of places in the code, each use-case can have their own
+ * prefix for their config. The caller should call {@link PinotConfiguration#subset(String)} to remove their prefix
+ * and this builder will look for exact matches of its relevant configs.
+ */
+ public static Builder newBuilder(PinotConfiguration pinotConfiguration) {
+ Builder builder = new Builder();
+ String maxConns = pinotConfiguration.getProperty(MAX_CONNS_CONFIG_NAME);
+ if (StringUtils.isNotEmpty(maxConns)) {
+ builder.withMaxConns(Integer.parseInt(maxConns));
+ }
+ String maxConnsPerRoute = pinotConfiguration.getProperty(MAX_CONNS_PER_ROUTE_CONFIG_NAME);
+ if (StringUtils.isNotEmpty(maxConnsPerRoute)) {
+ builder.withMaxConnsPerRoute(Integer.parseInt(maxConnsPerRoute));
+ }
+ return builder;
+ }
+
+ private static Builder newBuilder() {
+ return new Builder();
+ }
+
+ public static class Builder {
+ private int _maxConns = 20;
+ private int _maxConnsPerRoute = 2;
Review Comment:
I prefer the second approach in case the default changes in the `HttpClient`. By reading the `HttpClient` code, if not explicitly specified, these 2 values are 0, and then the underlying default is used
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] ankitsultana commented on a diff in pull request #10487: Fix Bottleneck for Server Bootstrap by Making maxConnsPerRoute Configurable
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #10487:
URL: https://github.com/apache/pinot/pull/10487#discussion_r1155245055
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/http/HttpClient.java:
##########
@@ -86,22 +87,23 @@ public class HttpClient implements AutoCloseable {
private final CloseableHttpClient _httpClient;
public HttpClient() {
- this(null);
+ this(HttpClientConfig.DEFAULT_HTTP_CLIENT_CONFIG, null);
}
- public HttpClient(@Nullable SSLContext sslContext) {
+ public HttpClient(HttpClientConfig httpClientConfig, @Nullable SSLContext sslContext) {
Review Comment:
Raised: https://github.com/apache/pinot/pull/10519
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org