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