kotman12 commented on code in PR #3357:
URL: https://github.com/apache/solr/pull/3357#discussion_r2129941025
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderBase.java:
##########
@@ -115,38 +121,49 @@ public B withMaxConnectionsPerHost(int max) {
return (B) this;
}
+ /**
+ * The max time a connection can be idle (that is, without traffic of bytes
in either direction).
+ * Sometimes called a "socket timeout". Zero means infinite. Note: not
applicable to the JDK
+ * HttpClient.
+ */
@SuppressWarnings("unchecked")
public B withIdleTimeout(long idleConnectionTimeout, TimeUnit unit) {
this.idleTimeoutMillis =
TimeUnit.MILLISECONDS.convert(idleConnectionTimeout, unit);
return (B) this;
}
- public Long getIdleTimeoutMillis() {
- return idleTimeoutMillis;
+ public long getIdleTimeoutMillis() {
+ return idleTimeoutMillis != null
Review Comment:
Wasn't the original logic, effectively:
`return idleTimeoutMillis != null && idleTimeoutMillis > 0 ?
idleTimeoutMillis : HttpClientUtil.DEFAULT_SO_TIMEOUT`
?
In general I think we should be very careful about default-setting the idle
timeout to "forever". I believe that in a majority of cases you _don't_ want
this default.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderBase.java:
##########
@@ -115,38 +121,49 @@ public B withMaxConnectionsPerHost(int max) {
return (B) this;
}
+ /**
+ * The max time a connection can be idle (that is, without traffic of bytes
in either direction).
+ * Sometimes called a "socket timeout". Zero means infinite. Note: not
applicable to the JDK
+ * HttpClient.
+ */
@SuppressWarnings("unchecked")
public B withIdleTimeout(long idleConnectionTimeout, TimeUnit unit) {
this.idleTimeoutMillis =
TimeUnit.MILLISECONDS.convert(idleConnectionTimeout, unit);
return (B) this;
}
- public Long getIdleTimeoutMillis() {
- return idleTimeoutMillis;
+ public long getIdleTimeoutMillis() {
+ return idleTimeoutMillis != null
+ ? (idleTimeoutMillis > 0 ? idleTimeoutMillis : FOREVER_MILLIS)
+ : HttpClientUtil.DEFAULT_SO_TIMEOUT;
}
+ /** The max time a connection can take to connect to destinations. Zero
means infinite. */
@SuppressWarnings("unchecked")
public B withConnectionTimeout(long connectionTimeout, TimeUnit unit) {
this.connectionTimeoutMillis =
TimeUnit.MILLISECONDS.convert(connectionTimeout, unit);
return (B) this;
}
- public Long getConnectionTimeout() {
- return connectionTimeoutMillis;
+ public long getConnectionTimeoutMillis() {
+ return connectionTimeoutMillis != null
+ ? (connectionTimeoutMillis > 0 ? connectionTimeoutMillis :
FOREVER_MILLIS)
+ : HttpClientUtil.DEFAULT_CONNECT_TIMEOUT;
}
- /**
- * Set a timeout in milliseconds for requests issued by this client.
- *
- * @param requestTimeout The timeout in milliseconds
- * @return this Builder.
- */
+ /** Set a timeout for requests to receive a response. Zero means infinite. */
@SuppressWarnings("unchecked")
public B withRequestTimeout(long requestTimeout, TimeUnit unit) {
this.requestTimeoutMillis = TimeUnit.MILLISECONDS.convert(requestTimeout,
unit);
return (B) this;
}
+ public long getRequestTimeoutMillis() {
+ return requestTimeoutMillis != null && requestTimeoutMillis >= 0
Review Comment:
Curious why setting a negative value defaults to the idle timeout but 0
means no timeout? I guess this is to preserve the existing behavior in Solr?
The underlying Jetty http request API has `<=0` signify no timeout.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -1016,28 +1017,31 @@ protected <B extends HttpSolrClientBase> B
build(Class<B> type) {
@Override
public Http2SolrClient build() {
- if (sslConfig == null) {
- sslConfig = Http2SolrClient.defaultSSLConfig;
- }
- if (cookieStore == null) {
- cookieStore = getDefaultCookieStore();
- }
- if (idleTimeoutMillis == null || idleTimeoutMillis <= 0) {
- idleTimeoutMillis = (long) HttpClientUtil.DEFAULT_SO_TIMEOUT;
- }
- if (connectionTimeoutMillis == null) {
- connectionTimeoutMillis = (long)
HttpClientUtil.DEFAULT_CONNECT_TIMEOUT;
- }
-
- if (keyStoreReloadIntervalSecs != null
- && keyStoreReloadIntervalSecs > 0
- && this.httpClient != null) {
- log.warn("keyStoreReloadIntervalSecs can't be set when using external
httpClient");
- keyStoreReloadIntervalSecs = null;
- } else if (keyStoreReloadIntervalSecs == null
- && this.httpClient == null
- && Boolean.getBoolean("solr.keyStoreReload.enabled")) {
- keyStoreReloadIntervalSecs =
Long.getLong("solr.jetty.sslContext.reload.scanInterval", 30);
+ if (httpClient == null) {
+ // set defaults for building an httpClient...
+ if (sslConfig == null) {
+ sslConfig = Http2SolrClient.defaultSSLConfig;
+ }
+ if (cookieStore == null) {
+ cookieStore = getDefaultCookieStore();
+ }
+ if (keyStoreReloadIntervalSecs == null
+ && Boolean.getBoolean("solr.keyStoreReload.enabled")) {
+ keyStoreReloadIntervalSecs =
+ Long.getLong("solr.jetty.sslContext.reload.scanInterval", 30);
+ }
+ } else {
+ if (followRedirects != null
+ || connectionTimeoutMillis != null
+ || maxConnectionsPerHost != null
+ || useHttp1_1 != httpClient.getTransport() instanceof
HttpClientTransportOverHTTP
+ || proxyHost != null
+ || sslConfig != null
+ || cookieStore != null
+ || keyStoreReloadIntervalSecs != null) {
+ throw new IllegalArgumentException(
Review Comment:
:+1:
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/SolrClientCache.java:
##########
@@ -205,19 +205,13 @@ private static Http2SolrClient.Builder
newHttp2SolrClientBuilder(
.withDefaultCollection(URLUtil.extractCoreFromCoreUrl(url));
if (http2SolrClient != null) {
builder.withHttpClient(http2SolrClient);
+ // cannot set connection timeout
Review Comment:
Should we validate that the `http2SolrClient` connectionTimeout is greater
than or equal to the `minConnTimeout`? Maybe this validation should be in the
constructor? There seems to be an expectation from the comments that this is
the "floor" for some reason... But if this is actually irrelevant then does it
make more sense to just rip out minConnTimeout altogether to avoid confusion?
--
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]
