Skip to content

Commit

Permalink
Allow deprecation warning of master_timeout parameter in HLRC
Browse files Browse the repository at this point in the history
Signed-off-by: Tianli Feng <ftianli@amazon.com>
  • Loading branch information
Tianli Feng committed Apr 15, 2022
1 parent e9f775d commit b0d588e
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 31 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1756,7 +1756,7 @@ private <Req, Resp> Resp internalPerformRequest(
Set<Integer> ignores
) throws IOException {
Request req = requestConverter.apply(request);
req.setOptions(options);
req.setOptions(allowMasterTimeoutWarning(options));
Response response;
try {
response = client.performRequest(req);
Expand Down Expand Up @@ -1797,7 +1797,7 @@ protected final <Req extends Validatable, Resp> Optional<Resp> performRequestAnd
throw validationException.get();
}
Request req = requestConverter.apply(request);
req.setOptions(options);
req.setOptions(allowMasterTimeoutWarning(options));
Response response;
try {
response = client.performRequest(req);
Expand Down Expand Up @@ -1922,7 +1922,7 @@ private <Req, Resp> Cancellable internalPerformRequestAsync(
listener.onFailure(e);
return Cancellable.NO_OP;
}
req.setOptions(options);
req.setOptions(allowMasterTimeoutWarning(options));

ResponseListener responseListener = wrapResponseListener(responseConverter, listener, ignores);
return client.performRequestAsync(req, responseListener);
Expand Down Expand Up @@ -2178,4 +2178,27 @@ static List<NamedXContentRegistry.Entry> getProvidedNamedXContents() {
}
return entries;
}

protected static final String MASTER_TIMEOUT_DEPRECATED_MESSAGE =
"Parameter [master_timeout] is deprecated and will be removed in 3.0. To support inclusive language, please use [cluster_manager_timeout] instead.";

protected RequestOptions allowMasterTimeoutWarning(RequestOptions options) {
WarningsHandler originalWarningsHandler = options.getWarningsHandler();
RequestOptions.Builder optionsBuilder = options.toBuilder();
optionsBuilder.setWarningsHandler(new WarningsHandler() {
@Override
public boolean warningsShouldFailRequest(List<String> warnings) {
if (originalWarningsHandler.warningsShouldFailRequest(Collections.singletonList(MASTER_TIMEOUT_DEPRECATED_MESSAGE))) {
warnings.remove(MASTER_TIMEOUT_DEPRECATED_MESSAGE);
}
return originalWarningsHandler.warningsShouldFailRequest(warnings);
}

@Override
public String toString() {
return originalWarningsHandler.toString();
}
});
return optionsBuilder.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@
import java.util.stream.Collectors;
import java.util.stream.Stream;

import static org.hamcrest.CoreMatchers.is;
import static org.opensearch.common.xcontent.XContentHelper.toXContent;
import static org.hamcrest.CoreMatchers.endsWith;
import static org.hamcrest.CoreMatchers.equalTo;
Expand Down Expand Up @@ -964,6 +965,27 @@ public void testApiNamingConventions() throws Exception {
assertThat("Some API are not supported but they should be: " + apiUnsupported, apiUnsupported.size(), equalTo(0));
}

public void testAllowMasterTimeoutWarning() throws IOException {
RequestOptions.Builder builder = RequestOptions.DEFAULT.toBuilder();
builder.setWarningsHandler(WarningsHandler.STRICT);
builder.addHeader("test", "value");
HttpAsyncResponseConsumerFactory consumerFactory = new HttpAsyncResponseConsumerFactory.HeapBufferedResponseConsumerFactory(1);
builder.setHttpAsyncResponseConsumerFactory(consumerFactory);
RequestOptions originalOptions = builder.build();

RequestOptions modifiedOptions = restHighLevelClient.allowMasterTimeoutWarning(originalOptions);
// The modified WarningsHandler should only allow the specific deprecation warning message.
List<String> warnings = new ArrayList<>();
warnings.add(RestHighLevelClient.MASTER_TIMEOUT_DEPRECATED_MESSAGE);
assertThat(modifiedOptions.getWarningsHandler().warningsShouldFailRequest(warnings), is(false));
warnings.add("another warning");
assertThat(modifiedOptions.getWarningsHandler().warningsShouldFailRequest(warnings), is(true));
assertThat(modifiedOptions.getWarningsHandler().toString(), is(WarningsHandler.STRICT.toString()));
// Other Options are not changed.
assertThat(modifiedOptions.getHeaders(), is(Collections.singletonList(new RequestOptions.ReqHeader("test", "value"))));
assertThat(modifiedOptions.getHttpAsyncResponseConsumerFactory(), is(consumerFactory));
}

private static void assertSyncMethod(Method method, String apiName, List<String> booleanReturnMethods) {
// A few methods return a boolean rather than a response object
if (apiName.equals("ping") || apiName.contains("exist") || booleanReturnMethods.contains(apiName)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,6 @@
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import static org.opensearch.rest.BaseRestHandler.MASTER_TIMEOUT_DEPRECATED_MESSAGE;

/**
* This is a simplistic logger that adds warning messages to HTTP headers.
* Use <code>HeaderWarning.addWarning(message,params)</code>. Message will be formatted according to RFC7234.
Expand Down Expand Up @@ -342,18 +340,6 @@ public static void addWarning(String message, Object... params) {
// package scope for testing
static void addWarning(Set<ThreadContext> threadContexts, String message, Object... params) {
final Iterator<ThreadContext> iterator = threadContexts.iterator();
/*
* As of 2.0, to support inclusive language, the REST API request parameter 'master_timeout' is deprecated.
* However, the specific deprecation warning message will not be added to HTTP response header.
* In Low Level REST Client, parameter 'master_timeout' is added to every applicable REST API call,
* see org.opensearch.client.RequestConverters.Params.withMasterTimeout(TimeValue).
* To keep the compatibility of Rest Client 2.x with server 1.x, the parameter 'master_timeout' is preserved.
* The deprecated parameter is not actively used by the user, so skip adding the warning message to HTTP header.
* TODO: Remove the 'if' statement after removing the REST API request parameter 'master_timeout'.
*/
if (message.equals(MASTER_TIMEOUT_DEPRECATED_MESSAGE)) {
return;
}
if (iterator.hasNext()) {
final String formattedMessage = LoggerMessageFormat.format(message, params);
final String warningHeaderValue = formatWarning(formattedMessage);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ protected Set<String> responseParams() {
return Collections.emptySet();
}

public static final String MASTER_TIMEOUT_DEPRECATED_MESSAGE =
protected static final String MASTER_TIMEOUT_DEPRECATED_MESSAGE =
"Parameter [master_timeout] is deprecated and will be removed in 3.0. To support inclusive language, please use [cluster_manager_timeout] instead.";
protected static final String DUPLICATE_PARAMETER_ERROR_MESSAGE =
"Please only use one of the request parameters [master_timeout, cluster_manager_timeout].";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,7 @@
import java.util.Set;
import java.util.stream.IntStream;

import static org.hamcrest.Matchers.nullValue;
import static org.opensearch.common.logging.HeaderWarning.WARNING_HEADER_PATTERN;
import static org.opensearch.rest.BaseRestHandler.MASTER_TIMEOUT_DEPRECATED_MESSAGE;
import static org.opensearch.test.hamcrest.RegexMatcher.matches;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
Expand Down Expand Up @@ -299,17 +297,6 @@ public void testWarningHeaderSizeSetting() throws IOException {
assertTrue(warningHeadersSize <= 1024);
}

/*
* Validate the deprecation warning message is not added to HTTP response header.
* TODO: Remove the test after removing the REST API request parameter 'master_timeout'.
*/
public void testMasterTimeoutDeprecationWarningNotAddToHttpHeader() {
ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
HeaderWarning.addWarning(Collections.singleton(threadContext), MASTER_TIMEOUT_DEPRECATED_MESSAGE);
final Map<String, List<String>> responseHeaders = threadContext.getResponseHeaders();
assertThat(responseHeaders.get("Warning"), nullValue());
}

private String range(int lowerInclusive, int upperInclusive) {
return IntStream.range(lowerInclusive, upperInclusive + 1)
.collect(StringBuilder::new, StringBuilder::appendCodePoint, StringBuilder::append)
Expand Down

0 comments on commit b0d588e

Please sign in to comment.