dsmiley commented on a change in pull request #1770:
URL: https://github.com/apache/lucene-solr/pull/1770#discussion_r486657172
##########
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.
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.
##########
File path:
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseCloudSolrClient.java
##########
@@ -926,136 +1092,191 @@ public RouteException(ErrorCode errorCode,
NamedList<Throwable> throwables, Map<
}
} // else: ??? how to set this ???
- NamedList<Object> resp = null;
- try {
- resp = sendRequest(request, inputCollections);
- //to avoid an O(n) operation we always add STATE_VERSION to the last and
try to read it from there
- Object o = resp == null || resp.size() == 0 ? null :
resp.get(STATE_VERSION, resp.size() - 1);
- if(o != null && o instanceof Map) {
- //remove this because no one else needs this and tests would fail if
they are comparing responses
- resp.remove(resp.size()-1);
- @SuppressWarnings({"rawtypes"})
- Map invalidStates = (Map) o;
- for (Object invalidEntries : invalidStates.entrySet()) {
- @SuppressWarnings({"rawtypes"})
- Map.Entry e = (Map.Entry) invalidEntries;
- getDocCollection((String) e.getKey(), (Integer) e.getValue());
- }
+ requestWithRetryOnStaleStateHelper(request, retryCount, inputCollections,
requestedCollections, isAdmin,
+ isAsyncRequest, apiFuture, currentFuture);
+ }
+ private void requestWithRetryOnStaleStateHelper(final SolrRequest<?> request,
Review comment:
it's not clear to me what is gained by refactoring out this method.
----------------------------------------------------------------
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
