Re: [PR] SOLR-17874: Stop using HttpSolrClient in production [solr]

["gerlowskija (via GitHub)" <gi...@apache.org>]

gerlowskija commented on code in PR #3501:
URL: https://github.com/apache/solr/pull/3501#discussion_r2316130508


##########
solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java:
##########
@@ -1028,7 +1038,7 @@ public Boolean call() throws Exception {
 
     @Override
     protected SolrResponse createResponse(NamedList<Object> namedList) {
-      return null;
+      return new SimpleSolrResponse();

Review Comment:
   [Q] What happens when this method returns 'null'?  Does changing this fix some bug that had been unnoticed?



##########
solr/core/src/java/org/apache/solr/handler/component/IterativeMergeStrategy.java:
##########
@@ -134,13 +121,4 @@ public Future<CallBack> callBack(ShardResponse response, QueryRequest req) {
   }
 
   protected abstract void process(ResponseBuilder rb, ShardRequest sreq) throws Exception;
-
-  private CloseableHttpClient getHttpClient() {
-    ModifiableSolrParams params = new ModifiableSolrParams();
-    params.set(HttpClientUtil.PROP_MAX_CONNECTIONS, 128);
-    params.set(HttpClientUtil.PROP_MAX_CONNECTIONS_PER_HOST, 32);

Review Comment:
   Yeah, this makes me a little leery.  Perhaps these properties were pick arbitrarily from the start.  But more likely they were set after someone hit a problem "out in the wild", and removing them allows a potential bug to creep back in.  IMO they're worth replicating or keeping around.



##########
solr/core/src/java/org/apache/solr/handler/admin/api/SyncShard.java:
##########
@@ -80,9 +80,9 @@ private void doSyncShard(String extCollectionName, String shardName)
     Replica leader = docCollection.getLeader(shardName);
 
     try (SolrClient client =
-        new HttpSolrClient.Builder(leader.getBaseUrl())
-            .withConnectionTimeout(15000, TimeUnit.MILLISECONDS)
-            .withSocketTimeout(60000, TimeUnit.MILLISECONDS)
+        new Http2SolrClient.Builder(leader.getBaseUrl())
+            .withHttpClient(coreContainer.getDefaultHttpSolrClient())
+            .withIdleTimeout(60000, TimeUnit.MILLISECONDS)

Review Comment:
   [Q] You've kept the "idle"/"read" timeout here, but dropped the connection timeout - is there a reason?
   
   In theory idt it should cause any problems: Solr would have to be seriously unhealthy to take 15s just to connect.  But a lot of these timeouts in the code are a result of issues that folks hit in the wild.  I worry a bit that changing timeout vals here and elsewhere will lead to folks hitting more timeout exceptions down the road, as much as the change makes sense on paper.



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