VishnuPriyaChandraSekar commented on code in PR #4626:
URL: https://github.com/apache/solr/pull/4626#discussion_r3883956901
##########
solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java:
##########
@@ -301,7 +301,7 @@ private HttpClient createHttpClient(Builder builder) {
asyncTracker.getMaxRequestsQueuedPerDestination());
httpClient.setUserAgentField(new HttpField(HttpHeader.USER_AGENT,
USER_AGENT));
httpClient.setConnectTimeout(builder.getConnectionTimeoutMillis());
- httpClient.setIdleTimeout(-1); // don't enforce an idle timeout at this
level
+ httpClient.setIdleTimeout(SolrHttpConstants.DEFAULT_SO_TIMEOUT);
Review Comment:
Here is my response for the concerns raised in the PR:
* A longer builder idle timeout might be capped - "We want to make it
possible to choose a longer idle timeout in the builder, I'd think. If a longer
one is chosen, it won't work; it'll be effectively capped at this amount."
* The longer builder idle timeout will not be capped. Jetty’s
Request.idleTimeout will override its global idle timeout (ie.,
httpClient.idleTimeout). The original global idle timeout will be put back once
the request completes.
* HTTP 2:
https://github.com/jetty/jetty.project/blob/jetty-12.1.x/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/internal/HttpSenderOverHTTP2.java#L203
* HTTP 1:
https://github.com/jetty/jetty.project/blob/jetty-12.1.x/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpConnectionOverHTTP.java#L419
* For example, if the global HttpClient idle timeout is 10 minutes
(currently hard-coded in HttpJettySolrClient) and Builder.idleTimeout is
configured to 20 minutes, the request will use a 20-minute idle timeout. This
means the request can remain idle for up to 20 minutes between data transfers,
while the global 10-minute timeout continues to apply at the connection level
outside of the request.
* Reusing an existing HttpClient should not prevent customization
* HttpJettySolrClientTest.testIdleTimeoutWithHttpClient checks whether
the existing HttpJettyClient could be reused with customization. I ran the test
on this change and it succeeds. This confirms that the change allows
customization.
* Long-lived streaming requests must continue to work
* Long-lived streaming requests are governed by both the request timeout
and the read timeout (i.e., idle timeout). To support long-running streams, the
request timeout is disabled by setting it to 0, while the read/idle timeout is
currently set to 10 minutes through httpClient.setIdleTimeout(). However, when
Request.idleTimeout is set, it takes precedence over the global HttpClient idle
timeout. This means clients can use builder.idleTimeout to configure the
maximum amount of time allowed between data transfers for an individual
streaming request. Given this, I have some second thoughts about hard-coding
the global idle timeout to 10 minutes. I explored the following options:
* Option 1: Hard-code a 10-minute global idle timeout:
This would
provide a reasonable default for long-lived streaming requests, assuming 10
minutes is sufficient for most clients. Clients could still configure a longer
or shorter request-level idle timeout through builder.idleTimeout. However, the
global idle timeout also serves as the connection idle timeout. A 10-minute
connection idle timeout may be too long, since it could allow unused
connections to remain idle for an unnecessarily long period.
* Option 2 (Recommended): Hard-code a 1-minute global idle timeout:
Use a shorter 1-minute global idle timeout to avoid keeping unused connections
alive for too long, while allowing clients to configure a longer request-level
idle timeout through builder.idleTimeout for long-running streaming requests.
This provides a shorter default timeout for idle connections while still giving
clients control over how long an individual streaming request can remain idle
between data transfers.
* The removed comment is still partially relevant
* Re-checking the [Jira
ticket](https://issues.apache.org/jira/browse/SOLR-17871) made me realize why
that comment existed—earlier changed the global idle timeout using
httpClient.setIdleTimeout(builder.getIdleTimeoutInMills()), which broke
connection reuse if the smaller idle timeout was used. Thus, it makes sense to
keep the builder’s idle timeout at the request level. I shall restore the
comment in the next revision
--
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]
