dsmiley commented on code in PR #3851:
URL: https://github.com/apache/solr/pull/3851#discussion_r2527747852
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -1300,9 +1306,19 @@ private CompletableFuture<DocCollection>
triggerCollectionRefresh(String collect
future.completeExceptionally(t);
}
} else {
Review Comment:
You're trying hard to load a collection's state in spite of a strong
indication that our CloudSolrClient is shutting down. Why bother? Imagine
instead simply assume threadPool is never null and proceed, leading to a
RejectedExecutionException (reasonable). Our close method needn't set
threadPool to null.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -1300,9 +1306,19 @@ private CompletableFuture<DocCollection>
triggerCollectionRefresh(String collect
future.completeExceptionally(t);
}
} else {
- future = CompletableFuture.supplyAsync(() ->
loadDocCollection(key), executor);
+ future =
+ CompletableFuture.supplyAsync(
+ () -> {
+ stateRefreshSemaphore.acquireUninterruptibly();
+ try {
+ return loadDocCollection(key);
+ } finally {
+ stateRefreshSemaphore.release();
+ }
+ },
+ executor);
}
- future.whenComplete(
+ future.whenCompleteAsync(
Review Comment:
This uses the default (ForkJoinPool) executor. Please pass the executor.
Maybe Solr should put the methods that use that executor on a forbidden-api
list?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
Review Comment:
I'm skeptical we should do any work whatsoever if we are closed.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -989,8 +989,9 @@ protected NamedList<Object> requestWithRetryOnStaleState(
}
}
- // First retry without sending state versions to avoid needless waits
when stale state is
- // still usable.
+ // First retry without sending state versions so the server does not
immediately reject the
Review Comment:
IMO we ought to improve the server at some point (not this issue/PR) to
*not* reject a request due to a non-synchronized stateVer. I filed
https://issues.apache.org/jira/browse/SOLR-17601 for that. I'll should add a
comment there that it should be able to simplify this code/complexity you are
adding here. Maybe you can add a reference to that JIRA here, please, to alert
the reader.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -1300,9 +1306,19 @@ private CompletableFuture<DocCollection>
triggerCollectionRefresh(String collect
future.completeExceptionally(t);
}
} else {
- future = CompletableFuture.supplyAsync(() ->
loadDocCollection(key), executor);
+ future =
+ CompletableFuture.supplyAsync(
+ () -> {
+ stateRefreshSemaphore.acquireUninterruptibly();
+ try {
+ return loadDocCollection(key);
+ } finally {
+ stateRefreshSemaphore.release();
+ }
+ },
+ executor);
}
- future.whenComplete(
+ future.whenCompleteAsync(
Review Comment:
Also; the task to do here is so trivially quick that I don't think it makes
sense to try to do asynchronously.
--
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]
