kotman12 commented on code in PR #3357:
URL: https://github.com/apache/solr/pull/3357#discussion_r2145271507
##########
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:
@dsmiley were you going to change this? Regarding:
> For example the SolrClientCache could then use max on and it'd behave
sensibly when compared to another default. If 0 means forever, it's
semantically inverted compared to the other values.
I think the existing logic does ensure that this returns greater than 0
because it defaults to idleTimeout in the case of `<=0`. Idle timeout's
original logic effectively was:
```
return idleTimeoutMillis != null && idleTimeoutMillis > 0 ?
idleTimeoutMillis : HttpClientUtil.DEFAULT_SO_TIMEOUT
```
so again, unless `HttpClientUtil.DEFAULT_SO_TIMEOUT` returns a value `<=0`
(which should never be the case) I don't see how you would ever be inverting
the meanings of these values and so I think it would be safe to call `max` when
comparing them.
The current approach in this PR is more complicated because it does
double-defaulting:
```
{
timeout < 0: idleTimeout
timeout = 0: Integer.MAX_VALUE
timeout > 0: timeout
}
```
I don't think this is really useful? Maybe I'm still missing something.
--
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]
