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: [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]
