[jira] [Commented] (SOLR-13605) HttpSolrClient.Builder.withHttpClient() is useless for the purpose of setting client scoped so/connect timeouts

["Eric Pugh (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/SOLR-13605?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17711489#comment-17711489 ] 

Eric Pugh commented on SOLR-13605:
----------------------------------

[~dsmiley] shared this JIRA with me since I'd been working in this space.  Is there a suggestion on what SHOULD be done?  Should HttpSolrClient.Builder.withHttpClient actually set those two fields?   I could add a unit test and a fix if thats the direction we can go in.  Or..   Do we think the whole "withHttpClient" is broken, and maybe the fix is just to remove it????   WOuld just love some direction.

> HttpSolrClient.Builder.withHttpClient() is useless for the purpose of setting client scoped so/connect timeouts
> ---------------------------------------------------------------------------------------------------------------
>
>                 Key: SOLR-13605
>                 URL: https://issues.apache.org/jira/browse/SOLR-13605
>             Project: Solr
>          Issue Type: Bug
>            Reporter: Chris M. Hostetter
>            Priority: Major
>
> TL;DR: trying to use {{HttpSolrClient.Builder.withHttpClient}} is useless for the the purpose of specifying an {{HttpClient}} with the default "timeouts" you want to use on all requests, because of how {{HttpSolrClient.Builder}} and {{HttpClientUtil.createDefaultRequestConfigBuilder()}} hardcode values thta get set on every {{HttpRequest}}.
> This internally affects code that uses things like {{UpdateShardHandler.getDefaultHttpClient()}}, {{UpdateShardHandler.getUpdateOnlyHttpClient()}} {{UpdateShardHandler.getRecoveryOnlyHttpClient()}}, etc...
> ----
> While looking into the patch in SOLR-13532, I realized that the way {{HttpSolrClient.Builder}} and it's super class {{SolrClientBuilder}} work, the following code doesn't do what a reasonable person would expect...
> {code:java}
> SolrParams clientParams = params(HttpClientUtil.PROP_SO_TIMEOUT, 12345,
>                                  HttpClientUtil.PROP_CONNECTION_TIMEOUT, 67890);
> HttpClient httpClient = HttpClientUtil.createClient(clientParams);
> HttpSolrClient solrClient = new HttpSolrClient.Builder(ANY_BASE_SOLR_URL)
>         .withHttpClient(httpClient)
>         .build();
> {code}
> When {{solrClient}} is used to execute a request, neither of the properties passed to {{HttpClientUtil.createClient(...)}} will matter - the {{HttpSolrClient.Builder}} (via inheritence from {{SolrClientBuilder}} has the following hardcoded values...
> {code:java}
>   // SolrClientBuilder
>   protected Integer connectionTimeoutMillis = 15000;
>   protected Integer socketTimeoutMillis = 120000;
> {code}
> ...which unless overridden by calls to {{withConnectionTimeout()}} and {{withSocketTimeout()}} will get set on the {{HttpSolrClient}} object, and used on every request...
> {code:java}
>     // protected HttpSolrClient constructor
>     this.connectionTimeout = builder.connectionTimeoutMillis;
>     this.soTimeout = builder.socketTimeoutMillis;
> {code}
> It would be tempting to try and do something like this to work around the problem...
> {code:java}
> SolrParams clientParams = params(HttpClientUtil.PROP_SO_TIMEOUT, 12345,
>                                  HttpClientUtil.PROP_CONNECTION_TIMEOUT, 67890);
> HttpClient httpClient = HttpClientUtil.createClient(clientParams);
> HttpSolrClient solrClient = new HttpSolrClient.Builder(ANY_BASE_SOLR_URL)
>         .withHttpClient(httpClient)
>         .withSocketTimeout(null)
>         .withConnectionTimeout(null)
>         .build();
> {code}
> ...except for 2 problems:
>  # In {{HttpSolrClient.executeMethod}}, if the values of {{this.connectionTimeout}} or {{this.soTimeout}} are null, then the values from {{HttpClientUtil.createDefaultRequestConfigBuilder();}} get used, which has it's own hardcoded defaults.
>  # {{withSocketTimeout}} and {{withConnectionTimeout}} take an int, not a (nullable) Integer.
> So then maybe something like this would work? - particularly since at the {{HttpClient}} / {{HttpRequest}} / {{RequestConfig}} level, a "-1" set on the {{HttpRequest}}'s {{RequestConfig}} is suppose to mean "use the (client) default" ...
> {code:java}
> SolrParams clientParams = params(HttpClientUtil.PROP_SO_TIMEOUT, 12345,
>                                  HttpClientUtil.PROP_CONNECTION_TIMEOUT, 67890);
> HttpClient httpClient = HttpClientUtil.createClient(clientParams);
> HttpSolrClient client = new HttpSolrClient.Builder(ANY_BASE_SOLR_URL)
>         .withHttpClient(httpClient)
>         .withSocketTimeout(-1)
>         .withConnectionTimeout(-1)
>         .build();
> {code}
> ...except that if we do *that* we get an IllegalArgumentException...
> {code:java}
>   // SolrClientBuilder
>   public B withConnectionTimeout(int connectionTimeoutMillis) {
>     if (connectionTimeoutMillis < 0) {
>       throw new IllegalArgumentException("connectionTimeoutMillis must be a non-negative integer.");
>     }
> {code}
> This is madness, and eliminates most/all of the known value of using {{.withHttpClient}}



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org