dsmiley commented on code in PR #4626:
URL: https://github.com/apache/solr/pull/4626#discussion_r3554056172
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateJdkSolrClientTest.java:
##########
@@ -28,7 +28,11 @@ public HttpSolrClient solrClient(Integer
overrideIdleTimeoutMs) {
var builder =
new
HttpJdkSolrClient.Builder().withSSLContext(MockTrustManager.ALL_TRUSTING_SSL_CONTEXT);
if (overrideIdleTimeoutMs != null) {
- builder.withIdleTimeout(overrideIdleTimeoutMs, TimeUnit.MILLISECONDS);
+ builder
+ .withIdleTimeout(overrideIdleTimeoutMs, TimeUnit.MILLISECONDS)
+ // override the infinite request timeout with idle timeout to ensure
idle requests times
+ // out.
Review Comment:
Also I'm confused. You're implying that the idle timeout isn't honored if
the request timeout is infinite?
##########
changelog/unreleased/SOLR-17433-changed-default-request-timeout.yml:
##########
@@ -0,0 +1,31 @@
+# (DELETE ALL COMMENTS UP HERE AFTER FILLING THIS IN
Review Comment:
see this line ;-)
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/LB2SolrClientTest.java:
##########
@@ -405,6 +405,9 @@ private class TimeoutZombieTestContext implements
AutoCloseable {
new HttpJettySolrClient.Builder()
.withConnectionTimeout(1000, TimeUnit.MILLISECONDS)
.withIdleTimeout(1, TimeUnit.MILLISECONDS)
+ // override the infinite request timeout with idle timeout to
ensure idle requests
+ // times out.
Review Comment:
something seems suspicious... the idle timeout should work if the request
timeout is infinite. They are different things; idle refers to the longest
period of not receiving any data at all from the server. The request timeout
is an overall timeout for the entire request.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateJdkSolrClientTest.java:
##########
@@ -28,7 +28,11 @@ public HttpSolrClient solrClient(Integer
overrideIdleTimeoutMs) {
var builder =
new
HttpJdkSolrClient.Builder().withSSLContext(MockTrustManager.ALL_TRUSTING_SSL_CONTEXT);
if (overrideIdleTimeoutMs != null) {
- builder.withIdleTimeout(overrideIdleTimeoutMs, TimeUnit.MILLISECONDS);
+ builder
+ .withIdleTimeout(overrideIdleTimeoutMs, TimeUnit.MILLISECONDS)
+ // override the infinite request timeout with idle timeout to ensure
idle requests times
+ // out.
Review Comment:
use fewer words please (one line)
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateJdkSolrClientTest.java:
##########
@@ -28,7 +28,11 @@ public HttpSolrClient solrClient(Integer
overrideIdleTimeoutMs) {
var builder =
new
HttpJdkSolrClient.Builder().withSSLContext(MockTrustManager.ALL_TRUSTING_SSL_CONTEXT);
if (overrideIdleTimeoutMs != null) {
- builder.withIdleTimeout(overrideIdleTimeoutMs, TimeUnit.MILLISECONDS);
+ builder
+ .withIdleTimeout(overrideIdleTimeoutMs, TimeUnit.MILLISECONDS)
+ // override the infinite request timeout with idle timeout to ensure
idle requests times
+ // out.
+ .withRequestTimeout(overrideIdleTimeoutMs, TimeUnit.MILLISECONDS);
Review Comment:
This should probably just be 0, infinite?
--
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]
