rishisankar commented on a change in pull request #1770:
URL: https://github.com/apache/lucene-solr/pull/1770#discussion_r487588616
##########
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:
> This method signature has become too hard to understand. Taking both a
CompletableFuture "apiFuture" and AtomicReference to a CF "currentFuture" is
too complicated. Can you please devise some other scheme? Just one
CompletableFuture, or perhaps return the CF if that's sensible. I'm sure you
had your reasons, but the complexity factor for me is just too high.
Hmm the issue is that the user should receive a future before the request
and subsequent retries are processed. In that case, the apiFuture needs to be
created before the recursive method -- but the recursive method also needs to
have access to the apiFuture so that it can set the response/error once
requests/retries are complete. So I think that at the very least, apiFuture
probably needs to be an arg in the method signature.
I can think of a few other options for the currentFuture AtomicReference
though --
* The AtomicReference is only used for cancelling, i.e. from makeRequest
```
apiFuture.exceptionally((error) -> {
if (apiFuture.isCancelled()) {
currentFuture.get().cancel(true);
}
return null;
});
```
So perhaps, it may be easier to understand if it's changed to a
`AtomicReference<Runnable>` which stores a lambda exp that cancels the current
request's future.
* 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.
Do you think either of these would help with the code readability?
<br/>
> You might also consider changing the retry approach to switch from
recursion, to a special exception with a retryCount so the caller can do the
retry. I'm just throwing this out there as an idea that _might_ make the
overall code easier to understand -- not sure.
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).
----------------------------------------------------------------
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
