Harmonize SolrJ timeouts #3357
Harmonize SolrJ timeouts #3357
Conversation
There was a problem hiding this comment.
NOCOMMIT here; it remains to be seen how we'll solve the incompatibility of withHttpClient + with timeouts.
| responseListener.get(client.getIdleTimeout(), TimeUnit.MILLISECONDS); | ||
| responseListener.get(client.getRequestTimeoutMillis(), TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
It was weird to be using the idle (aka socket) timeout in a situation where we're waiting for the request.
| @@ -129,6 +126,12 @@ protected Http2SolrClient(String serverBaseUrl, Builder builder) { | |||
| super(serverBaseUrl, builder); | |||
|
|
|||
| if (builder.httpClient != null) { | |||
| if (builder.followRedirects != null | |||
There was a problem hiding this comment.
NOCOMMIT; it remains to be seen if we're going to retain HttpClient reuse but reusing it means we can't be customizing timeouts.
| @@ -261,8 +266,6 @@ private HttpClient createHttpClient(Builder builder) { | |||
| this.authenticationStore = new AuthenticationStoreHolder(); | |||
| httpClient.setAuthenticationStore(this.authenticationStore); | |||
|
|
|||
| httpClient.setConnectTimeout(builder.connectionTimeoutMillis); | |||
There was a problem hiding this comment.
just reorganized this upwards
| httpClient.setConnectTimeout(builder.getConnectionTimeoutMillis()); | ||
| httpClient.setIdleTimeout(builder.getIdleTimeoutMillis()); |
There was a problem hiding this comment.
note: these getters now resolve a default value, a primitive; do not return null.
| HttpClient.Builder b = HttpClient.newBuilder().followRedirects(followRedirects); | ||
| b.followRedirects(followRedirects); | ||
|
|
||
| b.connectTimeout(Duration.of(builder.getConnectionTimeoutMillis(), ChronoUnit.MILLIS)); | ||
| // note: idle timeout isn't used for the JDK client | ||
| // note: request timeout is set per request |
There was a problem hiding this comment.
- organizational change
- setting the connection timeout (was forgotten!)
| if (requestTimeoutMillis > 0) { | ||
| reqb.timeout(Duration.of(requestTimeoutMillis, ChronoUnit.MILLIS)); | ||
| } else if (idleTimeoutMillis > 0) { | ||
| reqb.timeout(Duration.of(idleTimeoutMillis, ChronoUnit.MILLIS)); | ||
| } | ||
| reqb.timeout(Duration.of(requestTimeoutMillis, ChronoUnit.MILLIS)); |
There was a problem hiding this comment.
same comment as for Http2SolrClient: we now keep this simply and do defaulting logic in one common place earlier
| if (idleTimeoutMillis == null || idleTimeoutMillis <= 0) { | ||
| idleTimeoutMillis = (long) HttpClientUtil.DEFAULT_SO_TIMEOUT; | ||
| } | ||
| if (connectionTimeoutMillis == null) { | ||
| connectionTimeoutMillis = (long) HttpClientUtil.DEFAULT_CONNECT_TIMEOUT; | ||
| } |
There was a problem hiding this comment.
same comment as for Http2SolrClient: these settings were redundant/duplicative and thus misleading. Only need to be inside the underlying HttpClient.
| @@ -57,8 +57,6 @@ public abstract class HttpSolrClientBase extends SolrClient { | |||
| /** The URL of the Solr server. */ | |||
| protected final String serverBaseUrl; | |||
|
|
|||
| protected final long idleTimeoutMillis; | |||
There was a problem hiding this comment.
redundant with HttpClient; removed here.
| .withHttpClient(oldClient) | ||
| .withIdleTimeout(3000, TimeUnit.MILLISECONDS) |
There was a problem hiding this comment.
can't do this pairing anymore
https://issues.apache.org/jira/browse/SOLR-XXXXX
https://lists.apache.org/thread/002ql2ol0dklswhskokhxopj3d9tpnwj