rishisankar commented on a change in pull request #1770:
URL: https://github.com/apache/lucene-solr/pull/1770#discussion_r474823050
##########
File path:
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java
##########
@@ -394,26 +393,26 @@ public void onHeaders(Response response) {
assert ObjectReleaseTracker.track(is);
try {
NamedList<Object> body = processErrorsAndResponse(solrRequest,
parser, response, is);
- asyncListener.onSuccess(body);
- } catch (RemoteSolrException e) {
- if (SolrException.getRootCause(e) != CANCELLED_EXCEPTION) {
- asyncListener.onFailure(e);
- }
- } catch (SolrServerException e) {
- asyncListener.onFailure(e);
+ future.complete(body);
+ } catch (RemoteSolrException | SolrServerException e) {
+ future.completeExceptionally(e);
}
});
}
@Override
public void onFailure(Response response, Throwable failure) {
super.onFailure(response, failure);
- if (failure != CANCELLED_EXCEPTION) {
- asyncListener.onFailure(new
SolrServerException(failure.getMessage(), failure));
- }
+ future.completeExceptionally(new
SolrServerException(failure.getMessage(), failure));
}
});
- return () -> req.abort(CANCELLED_EXCEPTION);
+ future.exceptionally((error) -> {
+ if (error instanceof CancellationException) {
+ req.abort(new Exception());
Review comment:
> * Why do an instanceof check on "error" at all; why not simply always
req.abort(error)?
> * Why create a new Exception when we already have a suitable Throwable
type in variable "error"?
Yes I think you're right, the instanceof check and new Exception seem
unneccessary - will change that
> I see your point, but I in this case from CompletableFuture perspective,
it get called with `cancel()` first then `completeExceptionally` will that mess
up with internal state of the future instance?
Hmm I may be misunderstanding the question but I don't think so - the
`future.exceptionally` is a hook that would run after the future is already
marked as completed due to an exception, so subsequent calls to
`completeExceptionally` shouldn't do anything. From CF javadoc, "When two or
more threads attempt to complete, completeExceptionally, or cancel a
CompletableFuture, only one of them succeeds."
----------------------------------------------------------------
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
