You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "dsmiley (via GitHub)" <gi...@apache.org> on 2025/08/23 04:59:43 UTC
[PR] SOLR-17874: Stop using HttpSolrClient in production [solr]
dsmiley opened a new pull request, #3501:
URL: https://github.com/apache/solr/pull/3501
Still using it in tests.
https://issues.apache.org/jira/browse/SOLR-17874
--
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
Re: [PR] SOLR-17874: Stop using HttpSolrClient in production [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3501:
URL: https://github.com/apache/solr/pull/3501#discussion_r2295117052
##########
solr/core/src/java/org/apache/solr/handler/admin/AdminHandlersProxy.java:
##########
@@ -134,9 +134,13 @@ public static Pair<Future<NamedList<Object>>, SolrClient> callRemoteNode(
throws IOException, SolrServerException {
log.debug("Proxying {} request to node {}", endpoint, nodeName);
URI baseUri = URI.create(zkController.zkStateReader.getBaseUrlForNodeName(nodeName));
- HttpSolrClient solr = new HttpSolrClient.Builder(baseUri.toString()).build();
+
+ var solr =
+ new Http2SolrClient.Builder(baseUri.toString())
+ .withHttpClient(zkController.getCoreContainer().getDefaultHttpSolrClient())
+ .build();
SolrRequest<?> proxyReq = new GenericSolrRequest(SolrRequest.METHOD.GET, endpoint, params);
- HttpSolrClient.HttpUriRequestResponse proxyResp = solr.httpUriRequest(proxyReq);
- return new Pair<>(proxyResp.future, solr);
+ var future = solr.requestAsync(proxyReq);
Review Comment:
one of the rare cases where we call `requestAsync`
##########
solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java:
##########
@@ -983,42 +992,41 @@ public SolrParams getParams() {
public Boolean call() throws Exception {
final RTimer timer = new RTimer();
int attempts = 0;
- try (HttpSolrClient solr =
- new HttpSolrClient.Builder(replica.getBaseUrl())
- .withDefaultCollection(replica.getCoreName())
- .build()) {
- // eventually, this loop will get killed by the ExecutorService's timeout
- while (true) {
- try {
- long timeElapsed = (long) timer.getTime() / 1000;
- if (timeElapsed >= maxWait) {
- return false;
- }
- log.info("Time elapsed : {} secs, maxWait {}", timeElapsed, maxWait);
- Thread.sleep(100);
- NamedList<Object> resp = solr.httpUriRequest(this).future.get();
Review Comment:
it was pointless to call an async method and immediately call get().
##########
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)
Review Comment:
I updated this to use `withHttpClient` and thus we can't configure the connection timeout. I'm skeptical the connection timeout needs customization in general.
##########
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:
admittedly I didn't create a new client in order to customize these things. Maybe I should?
##########
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())
Review Comment:
maybe should use the "update only" client? Hard to say.
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/CollectionHandlingUtils.java:
##########
@@ -238,19 +243,12 @@ static void commit(NamedList<Object> results, String slice, Replica parentShardL
}
}
- static UpdateResponse softCommit(String baseUrl, String coreName)
+ private static UpdateResponse softCommit(
+ Http2SolrClient solrClient, String baseUrl, String coreName)
throws SolrServerException, IOException {
-
- try (SolrClient client =
- new HttpSolrClient.Builder(baseUrl)
- .withDefaultCollection(coreName)
- .withConnectionTimeout(30000, TimeUnit.MILLISECONDS)
- .withSocketTimeout(120000, TimeUnit.MILLISECONDS)
Review Comment:
soft commit should be fast... but any way, it's now the caller that will pass in a suitable solrClient and in this case it'll be the "update" SolrClient.
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchema.java:
##########
@@ -372,38 +371,40 @@ public SolrParams getParams() {
@Override
public Integer call() throws Exception {
int remoteVersion = -1;
- try (HttpSolrClient solr =
- new HttpSolrClient.Builder(baseUrl).withDefaultCollection(coreName).build()) {
- // eventually, this loop will get killed by the ExecutorService's timeout
- while (remoteVersion == -1
- || (remoteVersion < expectedZkVersion
- && !zkController.getCoreContainer().isShutDown())) {
- try {
- HttpSolrClient.HttpUriRequestResponse mrr = solr.httpUriRequest(this);
- NamedList<Object> zkversionResp = mrr.future.get();
Review Comment:
same code pattern as elsewhere, same analysis: it's pointless to invoke an async method (httpUriRequest is asynchronous), only to immediately call `get` on 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
Re: [PR] SOLR-17874: Stop using HttpSolrClient in production [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #3501:
URL: https://github.com/apache/solr/pull/3501#issuecomment-3245599689
> I'd rather we use the same instance more than I think we should be waiting forever
Is creating these clients really that expensive/costly? My (possibly out-of-date) understanding was that we considered creation of the "Cloud" clients to be expensive, but that individual "Http" clients should be cheap to instantiate?
--
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
Re: [PR] SOLR-17874: Stop using HttpSolrClient in production [solr]
Posted by "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
Re: [PR] SOLR-17874: Stop using HttpSolrClient in production [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #3501:
URL: https://github.com/apache/solr/pull/3501#issuecomment-3245474636
Remember, we can't reuse an internal/default Http2SolrClient and yet also vary the connection timeout specifically (and some other obscure things). Jetty HttpClient does not support that; we'd have to create another one. I'd rather we use the same instance more than I think we should be waiting _forever_ to make an HTTP connection that really shouldn't take that long.
For `IterativeMergeStrategy` in particular, I defer to @joel-bernstein 's feedback who wrote that. I did spend some time staring at that thing... nothing within Solr is using it. I think it *should* have been designed to be more customizable so that a user can provide their own SolrClient if they wish.
--
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
Re: [PR] SOLR-17874: Stop using HttpSolrClient in production [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #3501:
URL: https://github.com/apache/solr/pull/3501#issuecomment-3226802286
This has been ready; I plan to merge in 24 hours.
CHANGES.txt would refer to an Other item something like:
* Switch remaining usages of Apache HttpClient to use the internally managed Jetty HttpClient instance.
--
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
Re: [PR] SOLR-17874: Stop using HttpSolrClient in production [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley merged PR #3501:
URL: https://github.com/apache/solr/pull/3501
--
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