Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove support for maxRetryTimeout from low-level REST client #38085

Merged
merged 16 commits into from
Feb 6, 2019
Merged

Remove support for maxRetryTimeout from low-level REST client #38085

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
5059a0b
Remove support for maxRetryTimeout from low-level REST client
javanna Jan 31, 2019
6a7f7cd
migrate note
javanna Jan 31, 2019
86fa432
remove unused argument
javanna Feb 1, 2019
29fd5a8
update javadocs
javanna Feb 1, 2019
c3a41bc
revisit sync calls to rely on future.get rather than countdownlatch
javanna Feb 4, 2019
2a2e92d
clarify migrate note
javanna Feb 4, 2019
9c26d70
rename internal response
javanna Feb 4, 2019
d938c03
expand tests for other exceptions
javanna Feb 4, 2019
be55d21
Merge branch 'master' into enhancement/remove_max_retry_timeout
javanna Feb 4, 2019
7dc26cc
adapt failing test
javanna Feb 5, 2019
ef67306
reintroduce exception wrapping and expand tests
javanna Feb 5, 2019
b4015e1
Merge branch 'master' into enhancement/remove_max_retry_timeout
javanna Feb 5, 2019
5812ecd
add wrapping for interrupted exception
javanna Feb 5, 2019
2a9e0d1
review
javanna Feb 5, 2019
ec0b055
fix failing test
javanna Feb 5, 2019
537ad09
Merge branch 'master' into enhancement/remove_max_retry_timeout
javanna Feb 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 87 additions & 46 deletions client/rest/src/main/java/org/elasticsearch/client/RestClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.apache.http.ConnectionClosedException;
import org.apache.http.Header;
import org.apache.http.HttpEntity;
import org.apache.http.HttpHost;
Expand All @@ -38,6 +39,7 @@
import org.apache.http.client.protocol.HttpClientContext;
import org.apache.http.client.utils.URIBuilder;
import org.apache.http.concurrent.FutureCallback;
import org.apache.http.conn.ConnectTimeoutException;
import org.apache.http.impl.auth.BasicScheme;
import org.apache.http.impl.client.BasicAuthCache;
import org.apache.http.impl.nio.client.CloseableHttpAsyncClient;
Expand All @@ -46,8 +48,11 @@
import org.apache.http.nio.protocol.HttpAsyncResponseConsumer;
import org.elasticsearch.client.DeadHostState.TimeSupplier;

import javax.net.ssl.SSLHandshakeException;
import java.io.Closeable;
import java.io.IOException;
import java.net.ConnectException;
import java.net.SocketTimeoutException;
import java.net.URI;
import java.net.URISyntaxException;
import java.util.ArrayList;
Expand Down Expand Up @@ -189,7 +194,12 @@ public List<Node> getNodes() {
* of them does, in which case an {@link IOException} will be thrown.
*
* This method works by performing an asynchronous call and waiting
* for its result.
* for the result. If the asynchronous call throws an exception we wrap
* it and rethrow it so that the stack trace attached to the exception
* contains the call site. While we attempt to preserve the original
* exception this isn't always possible and likely haven't covered all of
* the cases. You can get the original exception from
* {@link Exception#getCause()}.
*
* @param request the request to perform
* @return the response returned by Elasticsearch
Expand All @@ -212,66 +222,50 @@ private Response performRequest(final NodeTuple<Iterator<Node>> nodeTuple,
} catch(Exception e) {
RequestLogger.logFailedRequest(logger, request.httpRequest, context.node, e);
onFailure(context.node);
Exception cause = unwrapExecutionException(e);
Exception cause = extractAndWrapCause(e);
addSuppressedException(previousException, cause);
if (nodeTuple.nodes.hasNext()) {
return performRequest(nodeTuple, request, cause);
}
if (cause instanceof IOException) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh. I think you might be able to drop these instanceof if you moved some stuff into extractAndWrapCause. You already know the type. I'm not sure you need to do that though. It'd be slightly cleaner, I think, but it isn't worth holding up the PR for it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the thing is that there are cases where I don't re-throw the exception gotten from extractAndWrapCause, I may pass it over to performRequest is I can retry on anothe rnode :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ha! I see that two lines up. That makes sense.

throw (IOException)cause;
throw (IOException) cause;
}
if (cause instanceof RuntimeException) {
throw (RuntimeException)cause;
throw (RuntimeException) cause;
}
throw new RuntimeException(cause);
throw new IllegalStateException("cause must be either RuntimeException or IOException", cause);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "unexpected exception type so wrapping into an expected one to prevent even more chaos"?

}
ResponseOrException responseOrException = onResponse(request, context.node, httpResponse);
if (responseOrException.responseException == null) {
return responseOrException.response;
ResponseOrResponseException responseOrResponseException = convertResponse(request, context.node, httpResponse);
if (responseOrResponseException.responseException == null) {
return responseOrResponseException.response;
}
addSuppressedException(previousException, responseOrException.responseException);
addSuppressedException(previousException, responseOrResponseException.responseException);
if (nodeTuple.nodes.hasNext()) {
return performRequest(nodeTuple, request, responseOrException.responseException);
} else {
throw responseOrException.responseException;
}
}

private static Exception unwrapExecutionException(Exception e) {
if (e instanceof ExecutionException) {
ExecutionException executionException = (ExecutionException)e;
Throwable t = executionException.getCause() == null ? executionException : executionException.getCause();
if (t instanceof Error) {
throw (Error)t;
}
return (Exception)t;
return performRequest(nodeTuple, request, responseOrResponseException.responseException);
}
return e;
throw responseOrResponseException.responseException;
}

private ResponseOrException onResponse(InternalRequest request, Node node, HttpResponse httpResponse) throws IOException {
private ResponseOrResponseException convertResponse(InternalRequest request, Node node, HttpResponse httpResponse) throws IOException {
RequestLogger.logResponse(logger, request.httpRequest, node.getHost(), httpResponse);
int statusCode = httpResponse.getStatusLine().getStatusCode();
Response response = new Response(request.httpRequest.getRequestLine(), node.getHost(), httpResponse);
if (isSuccessfulResponse(statusCode) || request.ignoreErrorCodes.contains(response.getStatusLine().getStatusCode())) {
onResponse(node);
if (request.warningsHandler.warningsShouldFailRequest(response.getWarnings())) {
throw new WarningFailureException(response);
} else {
return new ResponseOrException(response);
}
} else {
ResponseException responseException = new ResponseException(response);
if (isRetryStatus(statusCode)) {
//mark host dead and retry against next one
onFailure(node);
return new ResponseOrException(responseException);
} else {
//mark host alive and don't retry, as the error should be a request problem
onResponse(node);
throw responseException;
}
return new ResponseOrResponseException(response);
}
ResponseException responseException = new ResponseException(response);
if (isRetryStatus(statusCode)) {
//mark host dead and retry against next one
onFailure(node);
return new ResponseOrResponseException(responseException);
}
//mark host alive and don't retry, as the error should be a request problem
onResponse(node);
throw responseException;
}

/**
Expand Down Expand Up @@ -308,15 +302,15 @@ private void performRequestAsync(final NodeTuple<Iterator<Node>> nodeTuple,
@Override
public void completed(HttpResponse httpResponse) {
try {
ResponseOrException responseOrException = onResponse(request, context.node, httpResponse);
if (responseOrException.responseException == null) {
listener.onSuccess(responseOrException.response);
ResponseOrResponseException responseOrResponseException = convertResponse(request, context.node, httpResponse);
if (responseOrResponseException.responseException == null) {
listener.onSuccess(responseOrResponseException.response);
} else {
if (nodeTuple.nodes.hasNext()) {
listener.trackFailure(responseOrException.responseException);
listener.trackFailure(responseOrResponseException.responseException);
performRequestAsync(nodeTuple, request, listener);
} else {
listener.onDefinitiveFailure(responseOrException.responseException);
listener.onDefinitiveFailure(responseOrResponseException.responseException);
}
}
} catch(Exception e) {
Expand Down Expand Up @@ -755,18 +749,65 @@ private static Set<Integer> getIgnoreErrorCodes(String ignoreString, String requ
return ignoreErrorCodes;
}

private static class ResponseOrException {
private static class ResponseOrResponseException {
private final Response response;
private final ResponseException responseException;

ResponseOrException(Response response) {
ResponseOrResponseException(Response response) {
this.response = Objects.requireNonNull(response);
this.responseException = null;
}

ResponseOrException(ResponseException responseException) {
ResponseOrResponseException(ResponseException responseException) {
this.responseException = Objects.requireNonNull(responseException);
this.response = null;
}
}

/**
* Wrap whatever exception we received, copying the type where possible so the synchronous API looks as much as possible
* like the asynchronous API. We wrap the exception so that the caller's signature shows up in any exception we throw.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd reverse the sentences. "Wrap the exception so the caller's signature shows up in the stack trace, taking care to copy the original type and message where possible so async and sync code don't have to check different exceptions."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or something like that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aware you may be copying a comment that I wrote and I'm effectively commenting on my own way of writing javadoc....

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

*/
private static Exception extractAndWrapCause(Exception exception) {
if (exception instanceof ExecutionException) {
ExecutionException executionException = (ExecutionException)exception;
Throwable t = executionException.getCause() == null ? executionException : executionException.getCause();
if (t instanceof Error) {
throw (Error)t;
}
exception = (Exception)t;
}
if (exception instanceof ConnectTimeoutException) {
ConnectTimeoutException e = new ConnectTimeoutException(exception.getMessage());
e.initCause(exception);
return e;
}
if (exception instanceof SocketTimeoutException) {
SocketTimeoutException e = new SocketTimeoutException(exception.getMessage());
e.initCause(exception);
return e;
}
if (exception instanceof ConnectionClosedException) {
ConnectionClosedException e = new ConnectionClosedException(exception.getMessage());
e.initCause(exception);
return e;
}
if (exception instanceof SSLHandshakeException) {
SSLHandshakeException e = new SSLHandshakeException(exception.getMessage());
e.initCause(exception);
return e;
}
if (exception instanceof ConnectException) {
ConnectException e = new ConnectException(exception.getMessage());
e.initCause(exception);
return e;
}
if (exception instanceof IOException) {
return new IOException(exception.getMessage(), exception);
}
if (exception instanceof RuntimeException){
return new RuntimeException(exception.getMessage(), exception);
}
return new RuntimeException("error while performing request", exception);
}
}
6 changes: 3 additions & 3 deletions client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsIntegTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ public void onFailure(Exception exception) {
* Test host selector against a real server <strong>and</strong>
* test what happens after calling
*/
public void testNodeSelector() throws IOException {
public void testNodeSelector() throws Exception {
try (RestClient restClient = buildRestClient(firstPositionNodeSelector())) {
Request request = new Request("GET", "/200");
int rounds = between(1, 10);
Expand All @@ -210,7 +210,7 @@ public void testNodeSelector() throws IOException {
*/
if (stoppedFirstHost) {
try {
restClient.performRequest(request);
RestClientSingleHostTests.performRequestSyncOrAsync(restClient, request);
fail("expected to fail to connect");
} catch (ConnectException e) {
// Windows isn't consistent here. Sometimes the message is even null!
Expand All @@ -219,7 +219,7 @@ public void testNodeSelector() throws IOException {
}
}
} else {
Response response = restClient.performRequest(request);
Response response = RestClientSingleHostTests.performRequestSyncOrAsync(restClient, request);
assertEquals(httpHosts[0], response.getHost());
}
}
Expand Down
Loading