You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "epugh (via GitHub)" <gi...@apache.org> on 2023/04/14 20:11:37 UTC
[GitHub] [solr] epugh opened a new pull request, #1565: SOLR-13605: HttpSolrClient.Builder.withHttpClient() is useless for the purpose of setting client scoped so/connect timeouts
epugh opened a new pull request, #1565:
URL: https://github.com/apache/solr/pull/1565
https://issues.apache.org/jira/browse/SOLR-13605
# Description
I have a unit test that demonstrates the problem. I don't know that I love it because I had to add some getters to HttpSolrClient... And, I don't know how to actually fix the problem. There is some commented out code that if we had a different type of HttpClient might work, but the version we have throws an `UnsupportedException`....
# Solution
Add a test to show the problem.. but no fix.
# Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1565: SOLR-13605: HttpSolrClient.Builder.withHttpClient() is useless for the purpose of setting client scoped so/connect timeouts
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1565:
URL: https://github.com/apache/solr/pull/1565#discussion_r1178220595
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderTest.java:
##########
@@ -51,6 +52,22 @@ public void testProvidesHttpClientToClient() throws IOException {
}
}
+ @AwaitsFix(bugUrl = "https://issues.apache.org/jira/browse/SOLR-13605")
+ @Test
+ public void testUsesTimeoutProvidedByHttpClient() throws IOException {
+
+ ModifiableSolrParams clientParams = new ModifiableSolrParams();
+ clientParams.set(HttpClientUtil.PROP_SO_TIMEOUT, 12345);
+ clientParams.set(HttpClientUtil.PROP_CONNECTION_TIMEOUT, 67890);
+ HttpClient httpClient = HttpClientUtil.createClient(clientParams);
+ try (HttpSolrClient createdClient =
+ new Builder(ANY_BASE_SOLR_URL).withHttpClient(httpClient).build()) {
+ assertEquals(createdClient.getHttpClient(), httpClient);
+ assertEquals(67890, createdClient.getConnectionTimeout());
+ assertEquals(12345, createdClient.getSocketTimeout());
+ }
+ }
Review Comment:
YOu are right... I added the close... One thing is that I think there are other tests that don't do this closing, like AdminHandlersProxyTest....
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1565: SOLR-13605: HttpSolrClient.Builder.withHttpClient() is useless for the purpose of setting client scoped so/connect timeouts
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1565:
URL: https://github.com/apache/solr/pull/1565#issuecomment-1523655098
I like your ideas @stillalex, do you want to try and push up a change? (Not sure if there are any permissions issue?)
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on pull request #1565: SOLR-13605: HttpSolrClient.Builder.withHttpClient() is useless for the purpose of setting client scoped so/connect timeouts
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on PR #1565:
URL: https://github.com/apache/solr/pull/1565#issuecomment-1523830570
sure @epugh will try to push something later today
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on pull request #1565: SOLR-13605: HttpSolrClient.Builder.withHttpClient() is useless for the purpose of setting client scoped so/connect timeouts
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on PR #1565:
URL: https://github.com/apache/solr/pull/1565#issuecomment-1526581097
@epugh that is way too much power :) could you pull the commit from my branch?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1565: SOLR-13605: HttpSolrClient.Builder.withHttpClient() is useless for the purpose of setting client scoped so/connect timeouts
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1565:
URL: https://github.com/apache/solr/pull/1565#discussion_r1178092451
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderTest.java:
##########
@@ -51,6 +52,22 @@ public void testProvidesHttpClientToClient() throws IOException {
}
}
+ @AwaitsFix(bugUrl = "https://issues.apache.org/jira/browse/SOLR-13605")
+ @Test
+ public void testUsesTimeoutProvidedByHttpClient() throws IOException {
+
+ ModifiableSolrParams clientParams = new ModifiableSolrParams();
+ clientParams.set(HttpClientUtil.PROP_SO_TIMEOUT, 12345);
+ clientParams.set(HttpClientUtil.PROP_CONNECTION_TIMEOUT, 67890);
+ HttpClient httpClient = HttpClientUtil.createClient(clientParams);
+ try (HttpSolrClient createdClient =
+ new Builder(ANY_BASE_SOLR_URL).withHttpClient(httpClient).build()) {
+ assertEquals(createdClient.getHttpClient(), httpClient);
+ assertEquals(67890, createdClient.getConnectionTimeout());
+ assertEquals(12345, createdClient.getSocketTimeout());
+ }
+ }
Review Comment:
i am trying to get it defined in the try/with/resources...
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh merged pull request #1565: SOLR-13605: HttpSolrClient.Builder.withHttpClient() is useless for the purpose of setting client scoped so/connect timeouts
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh merged PR #1565:
URL: https://github.com/apache/solr/pull/1565
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1565: SOLR-13605: HttpSolrClient.Builder.withHttpClient() is useless for the purpose of setting client scoped so/connect timeouts
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1565:
URL: https://github.com/apache/solr/pull/1565#issuecomment-1526331879
@stillalex since I'm hoping we collaborate a lot more, I tried to add you to my `epugh/solr` repo as a collaborator. I believe you should have an invite from me.. COuld you accept it and try and push to this PR? Would be great to confirm that we can both commit to a epugh/solr repo in the future!
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on pull request #1565: SOLR-13605: HttpSolrClient.Builder.withHttpClient() is useless for the purpose of setting client scoped so/connect timeouts
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on PR #1565:
URL: https://github.com/apache/solr/pull/1565#issuecomment-1524484701
@epugh I can't push to your branch directly, so I forked it to my own repo and pushed there. please take a look at https://github.com/stillalex/solr/commit/aa6ff0d6ec8982d8bd32d668fea478b6e9d130c3
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1565: SOLR-13605: HttpSolrClient.Builder.withHttpClient() is useless for the purpose of setting client scoped so/connect timeouts
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1565:
URL: https://github.com/apache/solr/pull/1565#discussion_r1178220595
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderTest.java:
##########
@@ -51,6 +52,22 @@ public void testProvidesHttpClientToClient() throws IOException {
}
}
+ @AwaitsFix(bugUrl = "https://issues.apache.org/jira/browse/SOLR-13605")
+ @Test
+ public void testUsesTimeoutProvidedByHttpClient() throws IOException {
+
+ ModifiableSolrParams clientParams = new ModifiableSolrParams();
+ clientParams.set(HttpClientUtil.PROP_SO_TIMEOUT, 12345);
+ clientParams.set(HttpClientUtil.PROP_CONNECTION_TIMEOUT, 67890);
+ HttpClient httpClient = HttpClientUtil.createClient(clientParams);
+ try (HttpSolrClient createdClient =
+ new Builder(ANY_BASE_SOLR_URL).withHttpClient(httpClient).build()) {
+ assertEquals(createdClient.getHttpClient(), httpClient);
+ assertEquals(67890, createdClient.getConnectionTimeout());
+ assertEquals(12345, createdClient.getSocketTimeout());
+ }
+ }
Review Comment:
YOu are right... I added the close... One thing is that I think there are other tests that don't do this closing, like AdminHandlersProxyTest.... THough on second look, I think we are referencing an existing httpClient in AdminHandlersProxyTest that maybe gets closed when the parent SolrClient gets closed..
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1565: SOLR-13605: HttpSolrClient.Builder.withHttpClient() is useless for the purpose of setting client scoped so/connect timeouts
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1565:
URL: https://github.com/apache/solr/pull/1565#issuecomment-1528789630
It worked @stillalex ... I added you as a remote, and was able to `cherry-pick` your commit!
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1565: SOLR-13605: HttpSolrClient.Builder.withHttpClient() is useless for the purpose of setting client scoped so/connect timeouts
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1565:
URL: https://github.com/apache/solr/pull/1565#discussion_r1178050642
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderTest.java:
##########
@@ -51,6 +52,22 @@ public void testProvidesHttpClientToClient() throws IOException {
}
}
+ @AwaitsFix(bugUrl = "https://issues.apache.org/jira/browse/SOLR-13605")
+ @Test
+ public void testUsesTimeoutProvidedByHttpClient() throws IOException {
+
+ ModifiableSolrParams clientParams = new ModifiableSolrParams();
+ clientParams.set(HttpClientUtil.PROP_SO_TIMEOUT, 12345);
+ clientParams.set(HttpClientUtil.PROP_CONNECTION_TIMEOUT, 67890);
+ HttpClient httpClient = HttpClientUtil.createClient(clientParams);
+ try (HttpSolrClient createdClient =
+ new Builder(ANY_BASE_SOLR_URL).withHttpClient(httpClient).build()) {
+ assertEquals(createdClient.getHttpClient(), httpClient);
+ assertEquals(67890, createdClient.getConnectionTimeout());
+ assertEquals(12345, createdClient.getSocketTimeout());
+ }
+ }
Review Comment:
missing
```
HttpClientUtil.close(httpClient);
```
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1565: SOLR-13605: HttpSolrClient.Builder.withHttpClient() is useless for the purpose of setting client scoped so/connect timeouts
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1565:
URL: https://github.com/apache/solr/pull/1565#discussion_r1178224029
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java:
##########
@@ -168,6 +168,12 @@ protected HttpSolrClient(Builder builder) {
this.internalClient = false;
this.followRedirects = builder.followRedirects;
this.httpClient = builder.httpClient;
+ // The getParams() is not supported by this httpClient type! Throws exception.
+ // this.soTimeout = builder.httpClient..getIntParameter(HttpClientUtil.PROP_SO_TIMEOUT,
+ // Math.toIntExact(builder.socketTimeoutMillis));
+ // this.connectionTimeout =
+ // builder.httpClient.getParams().getIntParameter(HttpClientUtil.PROP_CONNECTION_TIMEOUT,
+ // Math.toIntExact(builder.connectionTimeoutMillis));
Review Comment:
Can you push up an exampel of what you mean?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1565: SOLR-13605: HttpSolrClient.Builder.withHttpClient() is useless for the purpose of setting client scoped so/connect timeouts
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1565:
URL: https://github.com/apache/solr/pull/1565#discussion_r1178026053
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java:
##########
@@ -168,6 +168,12 @@ protected HttpSolrClient(Builder builder) {
this.internalClient = false;
this.followRedirects = builder.followRedirects;
this.httpClient = builder.httpClient;
+ // The getParams() is not supported by this httpClient type! Throws exception.
+ // this.soTimeout = builder.httpClient..getIntParameter(HttpClientUtil.PROP_SO_TIMEOUT,
+ // Math.toIntExact(builder.socketTimeoutMillis));
+ // this.connectionTimeout =
+ // builder.httpClient.getParams().getIntParameter(HttpClientUtil.PROP_CONNECTION_TIMEOUT,
+ // Math.toIntExact(builder.connectionTimeoutMillis));
Review Comment:
this can be
```
if(this.httpClient instanceof Configurable) {
RequestConfig conf = ((Configurable) httpClient).getConfig();
this.connectionTimeout = conf.getConnectTimeout();
this.soTimeout = conf.getSocketTimeout();
}
```
but also need to move the below from line 193 to the else branch on this if (line 185) so these 2 values only get set once
```
this.connectionTimeout = Math.toIntExact(builder.connectionTimeoutMillis);
this.soTimeout = Math.toIntExact(builder.socketTimeoutMillis);
```
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1565: SOLR-13605: HttpSolrClient.Builder.withHttpClient() is useless for the purpose of setting client scoped so/connect timeouts
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1565:
URL: https://github.com/apache/solr/pull/1565#discussion_r1178102320
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderTest.java:
##########
@@ -51,6 +52,22 @@ public void testProvidesHttpClientToClient() throws IOException {
}
}
+ @AwaitsFix(bugUrl = "https://issues.apache.org/jira/browse/SOLR-13605")
+ @Test
+ public void testUsesTimeoutProvidedByHttpClient() throws IOException {
+
+ ModifiableSolrParams clientParams = new ModifiableSolrParams();
+ clientParams.set(HttpClientUtil.PROP_SO_TIMEOUT, 12345);
+ clientParams.set(HttpClientUtil.PROP_CONNECTION_TIMEOUT, 67890);
+ HttpClient httpClient = HttpClientUtil.createClient(clientParams);
+ try (HttpSolrClient createdClient =
+ new Builder(ANY_BASE_SOLR_URL).withHttpClient(httpClient).build()) {
+ assertEquals(createdClient.getHttpClient(), httpClient);
+ assertEquals(67890, createdClient.getConnectionTimeout());
+ assertEquals(12345, createdClient.getSocketTimeout());
+ }
+ }
Review Comment:
interesting. HttpClient does not seem to implement AutoCloseable so you would not be able to do it like this. had no idea
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org