dsmiley commented on code in PR #3357:
URL: https://github.com/apache/solr/pull/3357#discussion_r2096418512
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java:
##########
@@ -553,12 +555,6 @@ public Builder(String baseSolrUrl) {
@Override
public HttpJdkSolrClient build() {
- if (idleTimeoutMillis == null || idleTimeoutMillis <= 0) {
- idleTimeoutMillis = (long) HttpClientUtil.DEFAULT_SO_TIMEOUT;
- }
- if (connectionTimeoutMillis == null) {
- connectionTimeoutMillis = (long)
HttpClientUtil.DEFAULT_CONNECT_TIMEOUT;
- }
Review Comment:
same comment as for Http2SolrClient: these settings were
redundant/duplicative and thus misleading. Only need to be inside the
underlying HttpClient.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java:
##########
@@ -419,11 +425,7 @@ private synchronized boolean
maybeTryHeadRequestSync(String url) {
}
private void decorateRequest(HttpRequest.Builder reqb, SolrRequest<?>
solrRequest) {
- 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));
Review Comment:
same comment as for Http2SolrClient: we now keep this simply and do
defaulting logic in one common place earlier
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java:
##########
@@ -57,8 +57,6 @@ public abstract class HttpSolrClientBase extends SolrClient {
/** The URL of the Solr server. */
protected final String serverBaseUrl;
- protected final long idleTimeoutMillis;
Review Comment:
redundant with HttpClient; removed here.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateHttp2SolrClient.java:
##########
@@ -256,7 +256,7 @@ void sendUpdateStream() throws Exception {
}
Response response =
- responseListener.get(client.getIdleTimeout(),
TimeUnit.MILLISECONDS);
+ responseListener.get(client.getRequestTimeoutMillis(),
TimeUnit.MILLISECONDS);
Review Comment:
It was weird to be using the idle (aka socket) timeout in a situation where
we're waiting for the request.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -261,8 +266,6 @@ private HttpClient createHttpClient(Builder builder) {
this.authenticationStore = new AuthenticationStoreHolder();
httpClient.setAuthenticationStore(this.authenticationStore);
- httpClient.setConnectTimeout(builder.connectionTimeoutMillis);
Review Comment:
just reorganized this upwards
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -1020,12 +1025,6 @@ public Http2SolrClient build() {
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;
- }
Review Comment:
No longer need such fields on the SolrClient since they are the
responsibility of the HttpClient, thus they were redundant here and misleading
(setting them changes nothing) IMO
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -514,7 +522,7 @@ public NamedList<Object> request(SolrRequest<?>
solrRequest, String collection)
try {
InputStreamResponseListener listener = new
InputStreamReleaseTrackingResponseListener();
req = sendRequest(makeRequest(solrRequest, url, false), listener);
- Response response = listener.get(idleTimeoutMillis,
TimeUnit.MILLISECONDS);
+ Response response = listener.get(requestTimeoutMillis,
TimeUnit.MILLISECONDS);
Review Comment:
was weird to be using the idle (AKA socket) timeout in a place where,
obviously, the request timeout is what should be used.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -252,7 +255,9 @@ private HttpClient createHttpClient(Builder builder) {
httpClient.setMaxRequestsQueuedPerDestination(
asyncTracker.getMaxRequestsQueuedPerDestination());
httpClient.setUserAgentField(new HttpField(HttpHeader.USER_AGENT,
USER_AGENT));
- httpClient.setIdleTimeout(idleTimeoutMillis);
+ httpClient.setConnectTimeout(builder.getConnectionTimeoutMillis());
+ httpClient.setIdleTimeout(builder.getIdleTimeoutMillis());
Review Comment:
note: these getters now resolve a default value, a primitive; do not return
null.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -622,11 +630,8 @@ private void setBasicAuthHeader(SolrRequest<?>
solrRequest, Request req) {
private void decorateRequest(Request req, SolrRequest<?> solrRequest,
boolean isAsync) {
req.headers(headers -> headers.remove(HttpHeader.ACCEPT_ENCODING));
- if (requestTimeoutMillis > 0) {
- req.timeout(requestTimeoutMillis, TimeUnit.MILLISECONDS);
- } else {
- req.timeout(idleTimeoutMillis, TimeUnit.MILLISECONDS);
- }
+ req.timeout(requestTimeoutMillis, TimeUnit.MILLISECONDS);
Review Comment:
We can & should keep this simple. there are 2 clients sharing some
foundation, and we now resolve a default early so here we just do something
obvious.
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/SolrClientCache.java:
##########
Review Comment:
NOCOMMIT here; it remains to be seen how we'll solve the incompatibility of
withHttpClient + with timeouts.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -1085,27 +1084,20 @@ public Builder withHttpClient(Http2SolrClient
http2SolrClient) {
if (this.basicAuthAuthorizationStr == null) {
this.basicAuthAuthorizationStr =
http2SolrClient.basicAuthAuthorizationStr;
}
- if (this.followRedirects == null) {
- this.followRedirects = http2SolrClient.httpClient.isFollowRedirects();
- }
- if (this.idleTimeoutMillis == null) {
- this.idleTimeoutMillis = http2SolrClient.idleTimeoutMillis;
+ if (this.requestTimeoutMillis == null) {
+ this.requestTimeoutMillis = http2SolrClient.requestTimeoutMillis;
Review Comment:
only the request timeout is owned by our SolrClient and *not* HttpClient
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -661,14 +661,16 @@ public void testIdleTimeoutWithHttpClient() {
assertEquals(oldClient.getIdleTimeout(),
onlyBaseUrlChangedClient.getIdleTimeout());
assertEquals(oldClient.getHttpClient(),
onlyBaseUrlChangedClient.getHttpClient());
}
- try (Http2SolrClient idleTimeoutChangedClient =
- new Http2SolrClient.Builder("baseSolrUrl")
- .withHttpClient(oldClient)
- .withIdleTimeout(3000, TimeUnit.MILLISECONDS)
Review Comment:
can't do this pairing anymore
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -129,6 +126,12 @@ protected Http2SolrClient(String serverBaseUrl, Builder
builder) {
super(serverBaseUrl, builder);
if (builder.httpClient != null) {
+ if (builder.followRedirects != null
Review 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.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java:
##########
@@ -86,12 +86,18 @@ public class HttpJdkSolrClient extends HttpSolrClientBase {
protected HttpJdkSolrClient(String serverBaseUrl, HttpJdkSolrClient.Builder
builder) {
super(serverBaseUrl, builder);
+ HttpClient.Builder b = HttpClient.newBuilder();
HttpClient.Redirect followRedirects =
Boolean.TRUE.equals(builder.followRedirects)
? HttpClient.Redirect.NORMAL
: HttpClient.Redirect.NEVER;
- 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
Review Comment:
* organizational change
* setting the connection timeout (was forgotten!)
--
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
