jdyer1 commented on code in PR #2899:
URL: https://github.com/apache/solr/pull/2899#discussion_r1889342889


##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -404,23 +400,6 @@ public Builder withCollectionCacheTtl(long timeToLive, 
TimeUnit unit) {
       return this;
     }
 
-    /**
-     * Set the internal http client.
-     *
-     * <p>Note: closing the httpClient instance is at the responsibility of 
the caller.
-     *
-     * @param httpClient http client
-     * @return this
-     */
-    public Builder withHttpClient(Http2SolrClient httpClient) {

Review Comment:
   This is the most controversial part of this PR, and I do not want to merge a 
potentially disruptive API change like this without buy-in.  I do not think 
this is much of a burden though, because the Builder has 
`withInternalClientBuilder`, which users should be able to easily migrate to 
use.  
   
   The purpose of forcing this is that `CloudHttp2SolrClient` in turn passes 
this Client(Builder) to `LBHttp2SolrClient`.  But as noted before,  
`LBHttp2SolrClient` doesn't really use this Client either; it clones it using 
`Http2SolrClient.builder#withHttpClient`.  Yet we've made  `LBHttp2SolrClient` 
generic so this means that any subclass of `HttpSolrClientBase` needs to also 
have a way to be cloned.  Personally I think clone methods like this are 
brittle and a maintenance burden, and I don't want to need to write one for 
`HttpJdkSolrClient`.  It also seems like a better API:  by telling the user we 
want a Builder, we signal that we will create Clients as needed.  Passing in an 
already-built Client signals that we will use that particular one, which is not 
always the case here.



-- 
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

Reply via email to