dsmiley commented on a change in pull request #1770:
URL: https://github.com/apache/lucene-solr/pull/1770#discussion_r489203281
##########
File path:
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseCloudSolrClient.java
##########
@@ -862,15 +1011,32 @@ public RouteException(ErrorCode errorCode,
NamedList<Throwable> throwables, Map<
}
List<String> inputCollections =
collection == null ? Collections.emptyList() :
StrUtils.splitSmart(collection, ",", true);
- return requestWithRetryOnStaleState(request, 0, inputCollections);
+
+ CompletableFuture<NamedList<Object>> apiFuture = new CompletableFuture<>();
+ final AtomicReference<CompletableFuture<NamedList<Object>>> currentFuture
= new AtomicReference<>();
+ apiFuture.exceptionally((error) -> {
+ if (apiFuture.isCancelled()) {
+ currentFuture.get().cancel(true);
+ }
+ return null;
+ });
+
+ requestWithRetryOnStaleState(request, 0, inputCollections, isAsyncRequest,
apiFuture, currentFuture);
+
+ return apiFuture;
}
/**
* As this class doesn't watch external collections on the client side,
* there's a chance that the request will fail due to cached stale state,
* which means the state must be refreshed from ZK and retried.
*/
- protected NamedList<Object>
requestWithRetryOnStaleState(@SuppressWarnings({"rawtypes"})SolrRequest
request, int retryCount, List<String> inputCollections)
+ protected void requestWithRetryOnStaleState(SolrRequest<?> request,
Review comment:
> Another option is to ditch currentFuture altogether, and add
apiFuture.exceptionally statements each time a request retry is sent out to
take care of cancelling that specific request's future. Only difference is
there will be one callback per request made instead of just one overall;
however, all but the most recent callback will be no-ops as their corresponding
request futures will have already failed (and spawned a retry request). And I
don't imagine there'd be too many retries so the number of callbacks shouldn't
cause any noticeable slowdown during cancellation.
That sounds like an acceptable trade-off. A comment could point out the
imperfection.
> Might be misunderstanding your suggestion, but assuming a request is made
asynchronously, the (api) future would be returned to the user before the
request (and potential request retries) are completed. Thus, I don't think the
caller can do the retry (as the caller will have finished execution before a
request fails and a retry is decided upon).
By "the caller", I still meant internally. Put differently, imagine if the
implementation was iterative instead of recursive before you came along to add
async. Maybe that would lead to something easier to understand/maintain? If
not then nevermind. It seems you've ruled it out so okay.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
