kotman12 commented on code in PR #3357:
URL: https://github.com/apache/solr/pull/3357#discussion_r2129982207
##########
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:
Actually if I am understanding the original correctly:
```
if (requestTimeoutMillis > 0) {
req.timeout(requestTimeoutMillis, TimeUnit.MILLISECONDS);
} else {
req.timeout(idleTimeoutMillis, TimeUnit.MILLISECONDS);
}
```
\+
```
if (builder.requestTimeoutMillis != null) {
this.requestTimeoutMillis = builder.requestTimeoutMillis;
} else {
this.requestTimeoutMillis = -1;
}
```
Then I'd expect the following to preserve existing behavior. If we want to
change existing behavior we should be very explicit about it and IMO we
shouldn't change it to something so complicated, i.e. we either stick with the
idle timeout fallback or we go to a "forever" fallback. I don't know if a
three-state default solution is really beneficial. At that point you'd want to
disambiguate by explicitly setting to "forever" or idleTimeout or whatever
else...
`return requestTimeoutMillis != null && requestTimeoutMillis > 0 ?
requestTimeoutMillis : getIdleTimeoutMillis()`
--
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]
