dsmiley commented on code in PR #3357:
URL: https://github.com/apache/solr/pull/3357#discussion_r2130726121


##########
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:
   I didn't intend to change existing behavior, but I wanted the getXXXTimeout 
methods to return a >0 value, so that it's  it's clear/obvious how to interpret 
it _and_ so that callers could do min/max against some other value sensibly.  
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.



-- 
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: issues-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org

Reply via email to