honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

LLRC: Make warning behavior pluggable per request (#36345) 

This allows you to plug the behavior that the LLRC uses to handle
warnings on a per request basis.

We entertained the idea of allowing you to set the warnings behavior to
strict mode on a per request basis but that wouldn't allow the high
level rest client to fail when it sees an unexpected warning.

We also entertained the idea of adding a list of "required warnings" to
the `RequestOptions` but that won't work well with failures that occur
*sometimes* like those we see in mixed clusters.

Adding a list of "allowed warnings" to the `RequestOptions` would work
for mixed clusters but it'd leave many of the assertions in our tests
weaker than we'd like.

This behavior plugging implementation allows us to make a "required
warnings" option when we need it and an "allowed warnings" behavior when
we need it.

I don't think this behavior is going to be commonly used by used outside
of the Elasticsearch build, but I expect they'll be a few commendably
paranoid folks who could use this behavior.
This commit is contained in:
Nik Everett 2018-12-10 08:32:00 -05:00 committed by GitHub
parent f79e602d9f
commit 9626e700ce
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 222 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

86
client/rest/src/main/java/org/elasticsearch/client/RequestOptions.java
View File

@ -24,31 +24,37 @@ import org.apache.http.Header;
import org.apache.http.nio.protocol.HttpAsyncResponseConsumer;
import org.elasticsearch.client.HttpAsyncResponseConsumerFactory.HeapBufferedResponseConsumerFactory;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Objects;
import java.util.ArrayList;
/**
* The portion of an HTTP request to Elasticsearch that can be
* manipulated without changing Elasticsearch's behavior.
*/
public final class RequestOptions {
/**
* Default request options.
*/
public static final RequestOptions DEFAULT = new Builder(
Collections.<Header>emptyList(), HeapBufferedResponseConsumerFactory.DEFAULT).build();
Collections.<Header>emptyList(), HeapBufferedResponseConsumerFactory.DEFAULT, null).build();
private final List<Header> headers;
private final HttpAsyncResponseConsumerFactory httpAsyncResponseConsumerFactory;
private final WarningsHandler warningsHandler;
private RequestOptions(Builder builder) {
this.headers = Collections.unmodifiableList(new ArrayList<>(builder.headers));
this.httpAsyncResponseConsumerFactory = builder.httpAsyncResponseConsumerFactory;
this.warningsHandler = builder.warningsHandler;
}
/**
* Create a builder that contains these options but can be modified.
*/
public Builder toBuilder() {
return new Builder(headers, httpAsyncResponseConsumerFactory);
return new Builder(headers, httpAsyncResponseConsumerFactory, warningsHandler);
}
/**
@ -68,12 +74,35 @@ public final class RequestOptions {
return httpAsyncResponseConsumerFactory;
}
/**
* How this request should handle warnings. If null (the default) then
* this request will default to the behavior dictacted by
* {@link RestClientBuilder#setStrictDeprecationMode}.
* <p>
* This can be set to {@link WarningsHandler#PERMISSIVE} if the client
* should ignore all warnings which is the same behavior as setting
* strictDeprecationMode to true. It can be set to
* {@link WarningsHandler#STRICT} if the client should fail if there are
* any warnings which is the same behavior as settings
* strictDeprecationMode to false.
* <p>
* It can also be set to a custom implementation of
* {@linkplain WarningsHandler} to permit only certain warnings or to
* fail the request if the warnings returned don't
* <strong>exactly</strong> match some set.
*/
public WarningsHandler getWarningsHandler() {
return warningsHandler;
}
@Override
public String toString() {
StringBuilder b = new StringBuilder();
b.append("RequestOptions{");
boolean comma = false;
if (headers.size() > 0) {
b.append(", headers=");
b.append("headers=");
comma = true;
for (int h = 0; h < headers.size(); h++) {
if (h != 0) {
b.append(',');
@ -82,7 +111,14 @@ public final class RequestOptions {
}
}
if (httpAsyncResponseConsumerFactory != HttpAsyncResponseConsumerFactory.DEFAULT) {
b.append(", consumerFactory=").append(httpAsyncResponseConsumerFactory);
if (comma) b.append(", ");
comma = true;
b.append("consumerFactory=").append(httpAsyncResponseConsumerFactory);
}
if (warningsHandler != null) {
if (comma) b.append(", ");
comma = true;
b.append("warningsHandler=").append(warningsHandler);
}
return b.append('}').toString();
}
@ -98,21 +134,30 @@ public final class RequestOptions {
RequestOptions other = (RequestOptions) obj;
return headers.equals(other.headers)
&& httpAsyncResponseConsumerFactory.equals(other.httpAsyncResponseConsumerFactory);
&& httpAsyncResponseConsumerFactory.equals(other.httpAsyncResponseConsumerFactory)
&& Objects.equals(warningsHandler, other.warningsHandler);
}
@Override
public int hashCode() {
return Objects.hash(headers, httpAsyncResponseConsumerFactory);
return Objects.hash(headers, httpAsyncResponseConsumerFactory, warningsHandler);
}
/**
* Builds {@link RequestOptions}. Get one by calling
* {@link RequestOptions#toBuilder} on {@link RequestOptions#DEFAULT} or
* any other {@linkplain RequestOptions}.
*/
public static class Builder {
private final List<Header> headers;
private HttpAsyncResponseConsumerFactory httpAsyncResponseConsumerFactory;
private WarningsHandler warningsHandler;
private Builder(List<Header> headers, HttpAsyncResponseConsumerFactory httpAsyncResponseConsumerFactory) {
private Builder(List<Header> headers, HttpAsyncResponseConsumerFactory httpAsyncResponseConsumerFactory,
WarningsHandler warningsHandler) {
this.headers = new ArrayList<>(headers);
this.httpAsyncResponseConsumerFactory = httpAsyncResponseConsumerFactory;
this.warningsHandler = warningsHandler;
}
/**
@ -141,6 +186,27 @@ public final class RequestOptions {
this.httpAsyncResponseConsumerFactory =
Objects.requireNonNull(httpAsyncResponseConsumerFactory, "httpAsyncResponseConsumerFactory cannot be null");
}
/**
* How this request should handle warnings. If null (the default) then
* this request will default to the behavior dictacted by
* {@link RestClientBuilder#setStrictDeprecationMode}.
* <p>
* This can be set to {@link WarningsHandler#PERMISSIVE} if the client
* should ignore all warnings which is the same behavior as setting
* strictDeprecationMode to true. It can be set to
* {@link WarningsHandler#STRICT} if the client should fail if there are
* any warnings which is the same behavior as settings
* strictDeprecationMode to false.
* <p>
* It can also be set to a custom implementation of
* {@linkplain WarningsHandler} to permit only certain warnings or to
* fail the request if the warnings returned don't
* <strong>exactly</strong> match some set.
*/
public void setWarningsHandler(WarningsHandler warningsHandler) {
this.warningsHandler = warningsHandler;
}
}
/**

11
client/rest/src/main/java/org/elasticsearch/client/RestClient.java
View File

@ -110,7 +110,7 @@ public class RestClient implements Closeable {
private final FailureListener failureListener;
private final NodeSelector nodeSelector;
private volatile NodeTuple<List<Node>> nodeTuple;
private final boolean strictDeprecationMode;
private final WarningsHandler warningsHandler;
RestClient(CloseableHttpAsyncClient client, long maxRetryTimeoutMillis, Header[] defaultHeaders, List<Node> nodes, String pathPrefix,
FailureListener failureListener, NodeSelector nodeSelector, boolean strictDeprecationMode) {
@ -120,7 +120,7 @@ public class RestClient implements Closeable {
this.failureListener = failureListener;
this.pathPrefix = pathPrefix;
this.nodeSelector = nodeSelector;
this.strictDeprecationMode = strictDeprecationMode;
this.warningsHandler = strictDeprecationMode ? WarningsHandler.STRICT : WarningsHandler.PERMISSIVE;
setNodes(nodes);
}
@ -275,11 +275,13 @@ public class RestClient implements Closeable {
FailureTrackingResponseListener failureTrackingResponseListener = new FailureTrackingResponseListener(listener);
long startTime = System.nanoTime();
performRequestAsync(startTime, nextNode(), httpRequest, ignoreErrorCodes,
request.getOptions().getWarningsHandler() == null ? warningsHandler : request.getOptions().getWarningsHandler(),
request.getOptions().getHttpAsyncResponseConsumerFactory(), failureTrackingResponseListener);
}
private void performRequestAsync(final long startTime, final NodeTuple<Iterator<Node>> nodeTuple, final HttpRequestBase request,
final Set<Integer> ignoreErrorCodes,
final WarningsHandler thisWarningsHandler,
final HttpAsyncResponseConsumerFactory httpAsyncResponseConsumerFactory,
final FailureTrackingResponseListener listener) {
final Node node = nodeTuple.nodes.next();
@ -298,7 +300,7 @@ public class RestClient implements Closeable {
Response response = new Response(request.getRequestLine(), node.getHost(), httpResponse);
if (isSuccessfulResponse(statusCode) || ignoreErrorCodes.contains(response.getStatusLine().getStatusCode())) {
onResponse(node);
if (strictDeprecationMode && response.hasWarnings()) {
if (thisWarningsHandler.warningsShouldFailRequest(response.getWarnings())) {
listener.onDefinitiveFailure(new ResponseException(response));
} else {
listener.onSuccess(response);
@ -343,7 +345,8 @@ public class RestClient implements Closeable {
} else {
listener.trackFailure(exception);
request.reset();
performRequestAsync(startTime, nodeTuple, request, ignoreErrorCodes, httpAsyncResponseConsumerFactory, listener);
performRequestAsync(startTime, nodeTuple, request, ignoreErrorCodes,
thisWarningsHandler, httpAsyncResponseConsumerFactory, listener);
}
} else {
listener.onDefinitiveFailure(exception);

53
client/rest/src/main/java/org/elasticsearch/client/WarningsHandler.java Normal file
View File

@ -0,0 +1,53 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.elasticsearch.client;
import java.util.List;
/**
* Called if there are warnings to determine if those warnings should fail the
* request.
*/
public interface WarningsHandler {
boolean warningsShouldFailRequest(List<String> warnings);
WarningsHandler PERMISSIVE = new WarningsHandler() {
@Override
public boolean warningsShouldFailRequest(List<String> warnings) {
return false;
}
@Override
public String toString() {
return "permissive";
}
};
WarningsHandler STRICT = new WarningsHandler() {
@Override
public boolean warningsShouldFailRequest(List<String> warnings) {
return false == warnings.isEmpty();
}
@Override
public String toString() {
return "strict";
}
};
}

15
client/rest/src/test/java/org/elasticsearch/client/RequestOptionsTests.java
View File

@ -118,6 +118,10 @@ public class RequestOptionsTests extends RestClientTestCase {
builder.setHttpAsyncResponseConsumerFactory(new HeapBufferedResponseConsumerFactory(1));
}
if (randomBoolean()) {
builder.setWarningsHandler(randomBoolean() ? WarningsHandler.STRICT : WarningsHandler.PERMISSIVE);
}
return builder;
}
@ -127,7 +131,7 @@ public class RequestOptionsTests extends RestClientTestCase {
private static RequestOptions mutate(RequestOptions options) {
RequestOptions.Builder mutant = options.toBuilder();
int mutationType = between(0, 1);
int mutationType = between(0, 2);
switch (mutationType) {
case 0:
mutant.addHeader("extra", "m");
@ -135,6 +139,15 @@ public class RequestOptionsTests extends RestClientTestCase {
case 1:
mutant.setHttpAsyncResponseConsumerFactory(new HeapBufferedResponseConsumerFactory(5));
return mutant.build();
case 2:
mutant.setWarningsHandler(new WarningsHandler() {
@Override
public boolean warningsShouldFailRequest(List<String> warnings) {
fail("never called");
return false;
}
});
return mutant.build();
default:
throw new UnsupportedOperationException("Unknown mutation type [" + mutationType + "]");
}

68
client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostTests.java
View File

@ -19,6 +19,8 @@
package org.elasticsearch.client;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.apache.http.Header;
import org.apache.http.HttpEntity;
import org.apache.http.HttpEntityEnclosingRequest;
@ -92,6 +94,7 @@ import static org.mockito.Mockito.when;
* Relies on a mock http client to intercept requests and return desired responses based on request path.
*/
public class RestClientSingleHostTests extends RestClientTestCase {
private static final Log logger = LogFactory.getLog(RestClientSingleHostTests.class);
private ExecutorService exec = Executors.newFixedThreadPool(1);
private RestClient restClient;
@ -347,7 +350,51 @@ public class RestClientSingleHostTests extends RestClientTestCase {
assertDeprecationWarnings(
Arrays.asList(formatWarning(chars), "another one", "and another"),
Arrays.asList(chars, "another one", "and another"));
assertDeprecationWarnings(
Arrays.asList("ignorable one", "and another"),
Arrays.asList("ignorable one", "and another"));
assertDeprecationWarnings(singletonList("exact"), singletonList("exact"));
assertDeprecationWarnings(Collections.<String>emptyList(), Collections.<String>emptyList());
}
private enum DeprecationWarningOption {
PERMISSIVE {
protected WarningsHandler warningsHandler() {
return WarningsHandler.PERMISSIVE;
}
},
STRICT {
protected WarningsHandler warningsHandler() {
return WarningsHandler.STRICT;
}
},
FILTERED {
protected WarningsHandler warningsHandler() {
return new WarningsHandler() {
@Override
public boolean warningsShouldFailRequest(List<String> warnings) {
for (String warning : warnings) {
if (false == warning.startsWith("ignorable")) {
return true;
}
}
return false;
}
};
}
},
EXACT {
protected WarningsHandler warningsHandler() {
return new WarningsHandler() {
@Override
public boolean warningsShouldFailRequest(List<String> warnings) {
return false == warnings.equals(Arrays.asList("exact"));
}
};
}
};
protected abstract WarningsHandler warningsHandler();
}
private void assertDeprecationWarnings(List<String> warningHeaderTexts, List<String> warningBodyTexts) throws IOException {
@ -357,22 +404,35 @@ public class RestClientSingleHostTests extends RestClientTestCase {
for (String warningHeaderText : warningHeaderTexts) {
options.addHeader("Warning", warningHeaderText);
}
final boolean expectFailure;
if (randomBoolean()) {
logger.info("checking strictWarningsMode=[" + strictDeprecationMode + "] and warnings=" + warningBodyTexts);
expectFailure = strictDeprecationMode && false == warningBodyTexts.isEmpty();
} else {
DeprecationWarningOption warningOption = randomFrom(DeprecationWarningOption.values());
logger.info("checking warningOption=" + warningOption + " and warnings=" + warningBodyTexts);
options.setWarningsHandler(warningOption.warningsHandler());
expectFailure = warningOption.warningsHandler().warningsShouldFailRequest(warningBodyTexts);
}
request.setOptions(options);
Response response;
if (strictDeprecationMode) {
if (expectFailure) {
try {
restClient.performRequest(request);
fail("expected ResponseException because strict deprecation mode is enabled");
fail("expected ResponseException from warnings");
return;
} catch (ResponseException e) {
assertThat(e.getMessage(), containsString("\nWarnings: " + warningBodyTexts));
if (false == warningBodyTexts.isEmpty()) {
assertThat(e.getMessage(), containsString("\nWarnings: " + warningBodyTexts));
}
response = e.getResponse();
}
} else {
response = restClient.performRequest(request);
}
assertTrue(response.hasWarnings());
assertEquals(false == warningBodyTexts.isEmpty(), response.hasWarnings());
assertEquals(warningBodyTexts, response.getWarnings());
}

5
test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java
View File

@ -29,6 +29,7 @@ import org.apache.http.util.EntityUtils;
import org.elasticsearch.Version;
import org.elasticsearch.action.admin.cluster.node.tasks.list.ListTasksAction;
import org.elasticsearch.client.Request;
import org.elasticsearch.client.RequestOptions;
import org.elasticsearch.client.Response;
import org.elasticsearch.client.ResponseException;
import org.elasticsearch.client.RestClient;
@ -618,7 +619,11 @@ public abstract class ESRestTestCase extends ESTestCase {
/**
* Whether the used REST client should return any response containing at
* least one warning header as a failure.
* @deprecated always run in strict mode and use
* {@link RequestOptions.Builder#setWarningsHandler} to override this
* behavior on individual requests
*/
@Deprecated
protected boolean getStrictDeprecationMode() {
return true;
}

3
test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestClient.java
View File

@ -35,6 +35,7 @@ import org.elasticsearch.client.Response;
import org.elasticsearch.client.ResponseException;
import org.elasticsearch.client.RestClient;
import org.elasticsearch.client.RestClientBuilder;
import org.elasticsearch.client.WarningsHandler;
import org.elasticsearch.common.CheckedSupplier;
import org.elasticsearch.test.rest.yaml.restspec.ClientYamlSuiteRestApi;
import org.elasticsearch.test.rest.yaml.restspec.ClientYamlSuiteRestPath;
@ -216,6 +217,8 @@ public class ClientYamlTestClient implements Closeable {
logger.debug("Adding header {} with value {}", header.getKey(), header.getValue());
options.addHeader(header.getKey(), header.getValue());
}
// We check the warnings ourselves so we don't need the client to do it for us
options.setWarningsHandler(WarningsHandler.PERMISSIVE);
request.setOptions(options);
}

5
test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ESClientYamlSuiteTestCase.java
View File

@ -432,9 +432,4 @@ public abstract class ESClientYamlSuiteTestCase extends ESRestTestCase {
configureClient(builder, restClientSettings());
return builder;
}
@Override
protected boolean getStrictDeprecationMode() {
return false;
}
}