Skip to content

Commit

Permalink
Introduce RestUtils#getMasterNodeTimeout
Browse files Browse the repository at this point in the history
Many APIs accept a `?master_timeout` parameter, but reading this
parameter requires a little unnecessary boilerplate to specify the
literal parameter name and default value. Moreover, today's convention
is to construct a `MasterNodeRequest` and then read the default master
timeout from the freshly-created request. In practice this results in a
default of 30s, but we specify in the docs that this default is _always_
30s, and in principle one could create a transport request with a
different initial value which would deviate from the documented
behaviour.

This commit introduces a utility method for reading this parameter in a
fashion which is completely consistent with the documented behaviour.

Relates elastic#107984
  • Loading branch information
DaveCTurner committed Apr 28, 2024
1 parent 507c1b6 commit 87ecde6
Show file tree
Hide file tree
Showing 142 changed files with 340 additions and 157 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import java.util.List;

import static org.elasticsearch.rest.RestRequest.Method.GET;
import static org.elasticsearch.rest.RestUtils.getMasterNodeTimeout;

@ServerlessScope(Scope.PUBLIC)
public class RestDataStreamLifecycleStatsAction extends BaseRestHandler {
Expand All @@ -36,7 +37,7 @@ public List<Route> routes() {
@Override
protected RestChannelConsumer prepareRequest(RestRequest restRequest, NodeClient client) {
GetDataStreamLifecycleStatsAction.Request request = new GetDataStreamLifecycleStatsAction.Request();
request.masterNodeTimeout(restRequest.paramAsTime("master_timeout", request.masterNodeTimeout()));
request.masterNodeTimeout(getMasterNodeTimeout(restRequest));
return channel -> client.execute(
GetDataStreamLifecycleStatsAction.INSTANCE,
request,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.util.List;

import static org.elasticsearch.rest.RestRequest.Method.GET;
import static org.elasticsearch.rest.RestUtils.getMasterNodeTimeout;

@ServerlessScope(Scope.PUBLIC)
public class RestExplainDataStreamLifecycleAction extends BaseRestHandler {
Expand All @@ -41,7 +42,7 @@ protected RestChannelConsumer prepareRequest(RestRequest restRequest, NodeClient
ExplainDataStreamLifecycleAction.Request explainRequest = new ExplainDataStreamLifecycleAction.Request(indices);
explainRequest.includeDefaults(restRequest.paramAsBoolean("include_defaults", false));
explainRequest.indicesOptions(IndicesOptions.fromRequest(restRequest, IndicesOptions.strictExpandOpen()));
explainRequest.masterNodeTimeout(restRequest.paramAsTime("master_timeout", explainRequest.masterNodeTimeout()));
explainRequest.masterNodeTimeout(getMasterNodeTimeout(restRequest));
return channel -> client.execute(
ExplainDataStreamLifecycleAction.INSTANCE,
explainRequest,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.List;

import static org.elasticsearch.rest.RestRequest.Method.PUT;
import static org.elasticsearch.rest.RestUtils.getMasterNodeTimeout;

@ServerlessScope(Scope.PUBLIC)
public class RestPutDataStreamLifecycleAction extends BaseRestHandler {
Expand All @@ -41,7 +42,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli
try (XContentParser parser = request.contentParser()) {
PutDataStreamLifecycleAction.Request putLifecycleRequest = PutDataStreamLifecycleAction.Request.parseRequest(parser);
putLifecycleRequest.indices(Strings.splitStringByCommaToArray(request.param("name")));
putLifecycleRequest.masterNodeTimeout(request.paramAsTime("master_timeout", putLifecycleRequest.masterNodeTimeout()));
putLifecycleRequest.masterNodeTimeout(getMasterNodeTimeout(request));
putLifecycleRequest.ackTimeout(request.paramAsTime("timeout", putLifecycleRequest.ackTimeout()));
putLifecycleRequest.indicesOptions(IndicesOptions.fromRequest(request, putLifecycleRequest.indicesOptions()));
return channel -> client.execute(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.List;

import static org.elasticsearch.rest.RestRequest.Method.POST;
import static org.elasticsearch.rest.RestUtils.getMasterNodeTimeout;

@ServerlessScope(Scope.PUBLIC)
public class RestModifyDataStreamsAction extends BaseRestHandler {
Expand All @@ -43,7 +44,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli
if (modifyDsRequest.getActions() == null || modifyDsRequest.getActions().isEmpty()) {
throw new IllegalArgumentException("no data stream actions specified, at least one must be specified");
}
modifyDsRequest.masterNodeTimeout(request.paramAsTime("master_timeout", modifyDsRequest.masterNodeTimeout()));
modifyDsRequest.masterNodeTimeout(getMasterNodeTimeout(request));
modifyDsRequest.ackTimeout(request.paramAsTime("timeout", modifyDsRequest.ackTimeout()));
return channel -> client.execute(ModifyDataStreamsAction.INSTANCE, modifyDsRequest, new RestToXContentListener<>(channel));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import static org.elasticsearch.cluster.metadata.IndexGraveyard.SETTING_MAX_TOMBSTONES;
import static org.elasticsearch.indices.IndicesService.WRITE_DANGLING_INDICES_INFO_SETTING;
import static org.elasticsearch.rest.RestStatus.ACCEPTED;
import static org.elasticsearch.rest.RestUtils.REST_MASTER_TIMEOUT_PARAM;
import static org.elasticsearch.test.XContentTestUtils.createJsonMapView;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.equalTo;
Expand Down Expand Up @@ -111,7 +112,7 @@ public void testDanglingIndicesCanBeImported() throws Exception {
importRequest.addParameter("accept_data_loss", "true");
// Ensure this parameter is accepted
importRequest.addParameter("timeout", "20s");
importRequest.addParameter("master_timeout", "20s");
importRequest.addParameter(REST_MASTER_TIMEOUT_PARAM, "20s");
final Response importResponse = restClient.performRequest(importRequest);
assertThat(importResponse.getStatusLine().getStatusCode(), equalTo(ACCEPTED.getStatus()));

Expand Down Expand Up @@ -147,7 +148,7 @@ public void testDanglingIndicesCanBeDeleted() throws Exception {
deleteRequest.addParameter("accept_data_loss", "true");
// Ensure these parameters is accepted
deleteRequest.addParameter("timeout", "20s");
deleteRequest.addParameter("master_timeout", "20s");
deleteRequest.addParameter(REST_MASTER_TIMEOUT_PARAM, "20s");
final Response deleteResponse = restClient.performRequest(deleteRequest);
assertThat(deleteResponse.getStatusLine().getStatusCode(), equalTo(ACCEPTED.getStatus()));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

import java.io.IOException;

import static org.elasticsearch.rest.RestUtils.REST_MASTER_TIMEOUT_PARAM;

public class RestClusterStateActionIT extends ESIntegTestCase {

@Override
Expand All @@ -22,7 +24,7 @@ protected boolean addMockHttpTransport() {

public void testInfiniteTimeOut() throws IOException {
final var request = new Request("GET", "/_cluster/state/none");
request.addParameter("master_timeout", "-1");
request.addParameter(REST_MASTER_TIMEOUT_PARAM, "-1");
getRestClient().performRequest(request);
}
}
24 changes: 24 additions & 0 deletions server/src/main/java/org/elasticsearch/rest/RestUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import org.elasticsearch.common.Strings;
import org.elasticsearch.core.Booleans;
import org.elasticsearch.core.TimeValue;

import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
Expand Down Expand Up @@ -256,4 +257,27 @@ public static Optional<String> extractTraceId(String traceparent) {
return traceparent != null && traceparent.length() >= 55 ? Optional.of(traceparent.substring(3, 35)) : Optional.empty();
}

/**
* The name of the common {@code ?master_timeout} query parameter.
*/
public static final String REST_MASTER_TIMEOUT_PARAM = "master_timeout";

/**
* The default value for the common {@code ?master_timeout} query parameter.
*/
public static final TimeValue REST_MASTER_TIMEOUT_DEFAULT = TimeValue.timeValueSeconds(30);

/**
* Extract the {@code ?master_timeout} parameter from the request, imposing the common default of {@code 30s} in case the parameter is
* missing.
*
* @param restRequest The request from which to extract the {@code ?master_timeout} parameter
* @return the timeout from the request, with a default of {@link #REST_MASTER_TIMEOUT_DEFAULT} ({@code 30s}) if the request does not
* specify the parameter
*/
public static TimeValue getMasterNodeTimeout(RestRequest restRequest) {
assert restRequest != null;
return restRequest.paramAsTime(REST_MASTER_TIMEOUT_PARAM, REST_MASTER_TIMEOUT_DEFAULT);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.List;

import static org.elasticsearch.rest.RestRequest.Method.POST;
import static org.elasticsearch.rest.RestUtils.getMasterNodeTimeout;

public class RestAddVotingConfigExclusionAction extends BaseRestHandler {
private static final TimeValue DEFAULT_TIMEOUT = TimeValue.timeValueSeconds(30L);
Expand Down Expand Up @@ -82,7 +83,7 @@ static AddVotingConfigExclusionsRequest resolveVotingConfigExclusionsRequest(fin
request.paramAsTime("timeout", DEFAULT_TIMEOUT)
);

return resolvedRequest.masterNodeTimeout(request.paramAsTime("master_timeout", resolvedRequest.masterNodeTimeout()));
return resolvedRequest.masterNodeTimeout(getMasterNodeTimeout(request));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.List;

import static org.elasticsearch.rest.RestRequest.Method.POST;
import static org.elasticsearch.rest.RestUtils.getMasterNodeTimeout;

/**
* Cleans up a repository
Expand All @@ -42,7 +43,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
String name = request.param("repository");
CleanupRepositoryRequest cleanupRepositoryRequest = new CleanupRepositoryRequest(name);
cleanupRepositoryRequest.ackTimeout(request.paramAsTime("timeout", cleanupRepositoryRequest.ackTimeout()));
cleanupRepositoryRequest.masterNodeTimeout(request.paramAsTime("master_timeout", cleanupRepositoryRequest.masterNodeTimeout()));
cleanupRepositoryRequest.masterNodeTimeout(getMasterNodeTimeout(request));
return channel -> client.admin().cluster().cleanupRepository(cleanupRepositoryRequest, new RestToXContentListener<>(channel));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import java.util.List;

import static org.elasticsearch.rest.RestRequest.Method.DELETE;
import static org.elasticsearch.rest.RestUtils.getMasterNodeTimeout;

public class RestClearVotingConfigExclusionsAction extends BaseRestHandler {

Expand All @@ -45,7 +46,7 @@ protected RestChannelConsumer prepareRequest(final RestRequest request, final No

static ClearVotingConfigExclusionsRequest resolveVotingConfigExclusionsRequest(final RestRequest request) {
final var resolvedRequest = new ClearVotingConfigExclusionsRequest();
resolvedRequest.masterNodeTimeout(request.paramAsTime("master_timeout", resolvedRequest.masterNodeTimeout()));
resolvedRequest.masterNodeTimeout(getMasterNodeTimeout(request));
resolvedRequest.setTimeout(resolvedRequest.masterNodeTimeout());
resolvedRequest.setWaitForRemoval(request.paramAsBoolean("wait_for_removal", resolvedRequest.getWaitForRemoval()));
return resolvedRequest;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.util.Map;

import static org.elasticsearch.rest.RestRequest.Method.PUT;
import static org.elasticsearch.rest.RestUtils.getMasterNodeTimeout;

/**
* Clones indices from one snapshot into another snapshot in the same repository
Expand Down Expand Up @@ -51,7 +52,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
request.param("target_snapshot"),
XContentMapValues.nodeStringArrayValue(source.getOrDefault("indices", Collections.emptyList()))
);
cloneSnapshotRequest.masterNodeTimeout(request.paramAsTime("master_timeout", cloneSnapshotRequest.masterNodeTimeout()));
cloneSnapshotRequest.masterNodeTimeout(getMasterNodeTimeout(request));
cloneSnapshotRequest.indicesOptions(IndicesOptions.fromMap(source, cloneSnapshotRequest.indicesOptions()));
return channel -> client.admin().cluster().cloneSnapshot(cloneSnapshotRequest, new RestToXContentListener<>(channel));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.function.Predicate;

import static org.elasticsearch.rest.RestRequest.Method.GET;
import static org.elasticsearch.rest.RestUtils.getMasterNodeTimeout;

@ServerlessScope(Scope.INTERNAL)
public class RestClusterGetSettingsAction extends BaseRestHandler {
Expand Down Expand Up @@ -64,7 +65,7 @@ public String getName() {

private static void setUpRequestParams(MasterNodeReadRequest<?> clusterRequest, RestRequest request) {
clusterRequest.local(request.paramAsBoolean("local", clusterRequest.local()));
clusterRequest.masterNodeTimeout(request.paramAsTime("master_timeout", clusterRequest.masterNodeTimeout()));
clusterRequest.masterNodeTimeout(getMasterNodeTimeout(request));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.util.Set;

import static org.elasticsearch.rest.RestRequest.Method.GET;
import static org.elasticsearch.rest.RestUtils.getMasterNodeTimeout;

@ServerlessScope(Scope.INTERNAL)
public class RestClusterHealthAction extends BaseRestHandler {
Expand Down Expand Up @@ -63,7 +64,7 @@ public static ClusterHealthRequest fromRequest(final RestRequest request) {
final ClusterHealthRequest clusterHealthRequest = new ClusterHealthRequest(indices);
clusterHealthRequest.indicesOptions(IndicesOptions.fromRequest(request, clusterHealthRequest.indicesOptions()));
clusterHealthRequest.local(request.paramAsBoolean("local", clusterHealthRequest.local()));
clusterHealthRequest.masterNodeTimeout(request.paramAsTime("master_timeout", clusterHealthRequest.masterNodeTimeout()));
clusterHealthRequest.masterNodeTimeout(getMasterNodeTimeout(request));
clusterHealthRequest.timeout(request.paramAsTime("timeout", clusterHealthRequest.timeout()));
String waitForStatus = request.param("wait_for_status");
if (waitForStatus != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@

import static org.elasticsearch.common.util.set.Sets.addToCopy;
import static org.elasticsearch.rest.RestRequest.Method.POST;
import static org.elasticsearch.rest.RestUtils.getMasterNodeTimeout;

@ServerlessScope(Scope.INTERNAL)
public class RestClusterRerouteAction extends BaseRestHandler {
Expand Down Expand Up @@ -98,7 +99,7 @@ public static ClusterRerouteRequest createRequest(RestRequest request) throws IO
clusterRerouteRequest.explain(request.paramAsBoolean("explain", clusterRerouteRequest.explain()));
clusterRerouteRequest.ackTimeout(request.paramAsTime("timeout", clusterRerouteRequest.ackTimeout()));
clusterRerouteRequest.setRetryFailed(request.paramAsBoolean("retry_failed", clusterRerouteRequest.isRetryFailed()));
clusterRerouteRequest.masterNodeTimeout(request.paramAsTime("master_timeout", clusterRerouteRequest.masterNodeTimeout()));
clusterRerouteRequest.masterNodeTimeout(getMasterNodeTimeout(request));
request.applyContentParser(parser -> PARSER.parse(parser, clusterRerouteRequest, null));
return clusterRerouteRequest;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import static java.util.Collections.singletonMap;
import static org.elasticsearch.common.util.set.Sets.addToCopy;
import static org.elasticsearch.rest.RestRequest.Method.GET;
import static org.elasticsearch.rest.RestUtils.getMasterNodeTimeout;

@ServerlessScope(Scope.INTERNAL)
public class RestClusterStateAction extends BaseRestHandler {
Expand Down Expand Up @@ -81,7 +82,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
final ClusterStateRequest clusterStateRequest = new ClusterStateRequest();
clusterStateRequest.indicesOptions(IndicesOptions.fromRequest(request, clusterStateRequest.indicesOptions()));
clusterStateRequest.local(request.paramAsBoolean("local", clusterStateRequest.local()));
clusterStateRequest.masterNodeTimeout(request.paramAsTime("master_timeout", clusterStateRequest.masterNodeTimeout()));
clusterStateRequest.masterNodeTimeout(getMasterNodeTimeout(request));
if (request.hasParam("wait_for_metadata_version")) {
clusterStateRequest.waitForMetadataVersion(request.paramAsLong("wait_for_metadata_version", 0));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.util.Set;

import static org.elasticsearch.rest.RestRequest.Method.PUT;
import static org.elasticsearch.rest.RestUtils.getMasterNodeTimeout;

@ServerlessScope(Scope.INTERNAL)
public class RestClusterUpdateSettingsAction extends BaseRestHandler {
Expand All @@ -45,9 +46,7 @@ public String getName() {
public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
final ClusterUpdateSettingsRequest clusterUpdateSettingsRequest = new ClusterUpdateSettingsRequest();
clusterUpdateSettingsRequest.ackTimeout(request.paramAsTime("timeout", clusterUpdateSettingsRequest.ackTimeout()));
clusterUpdateSettingsRequest.masterNodeTimeout(
request.paramAsTime("master_timeout", clusterUpdateSettingsRequest.masterNodeTimeout())
);
clusterUpdateSettingsRequest.masterNodeTimeout(getMasterNodeTimeout(request));
Map<String, Object> source;
try (XContentParser parser = request.contentParser()) {
source = parser.map();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

import static org.elasticsearch.rest.RestRequest.Method.POST;
import static org.elasticsearch.rest.RestRequest.Method.PUT;
import static org.elasticsearch.rest.RestUtils.getMasterNodeTimeout;

/**
* Creates a new snapshot
Expand All @@ -44,7 +45,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
String snapshot = request.param("snapshot");
CreateSnapshotRequest createSnapshotRequest = new CreateSnapshotRequest(repository, snapshot);
request.applyContentParser(p -> createSnapshotRequest.source(p.mapOrdered()));
createSnapshotRequest.masterNodeTimeout(request.paramAsTime("master_timeout", createSnapshotRequest.masterNodeTimeout()));
createSnapshotRequest.masterNodeTimeout(getMasterNodeTimeout(request));
createSnapshotRequest.waitForCompletion(request.paramAsBoolean("wait_for_completion", false));
return channel -> client.admin().cluster().createSnapshot(createSnapshotRequest, new RestToXContentListener<>(channel));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
import java.io.IOException;
import java.util.List;

import static org.elasticsearch.rest.RestUtils.getMasterNodeTimeout;

public class RestDeleteDesiredNodesAction extends BaseRestHandler {
@Override
public String getName() {
Expand All @@ -32,7 +34,7 @@ public List<Route> routes() {
@Override
protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException {
final AcknowledgedRequest.Plain deleteDesiredNodesRequest = new AcknowledgedRequest.Plain();
deleteDesiredNodesRequest.masterNodeTimeout(request.paramAsTime("master_timeout", deleteDesiredNodesRequest.masterNodeTimeout()));
deleteDesiredNodesRequest.masterNodeTimeout(getMasterNodeTimeout(request));
return restChannel -> client.execute(
TransportDeleteDesiredNodesAction.TYPE,
deleteDesiredNodesRequest,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.List;

import static org.elasticsearch.rest.RestRequest.Method.DELETE;
import static org.elasticsearch.rest.RestUtils.getMasterNodeTimeout;

/**
* Unregisters a repository
Expand All @@ -45,7 +46,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
String name = request.param("repository");
DeleteRepositoryRequest deleteRepositoryRequest = new DeleteRepositoryRequest(name);
deleteRepositoryRequest.ackTimeout(request.paramAsTime("timeout", deleteRepositoryRequest.ackTimeout()));
deleteRepositoryRequest.masterNodeTimeout(request.paramAsTime("master_timeout", deleteRepositoryRequest.masterNodeTimeout()));
deleteRepositoryRequest.masterNodeTimeout(getMasterNodeTimeout(request));
return channel -> client.admin()
.cluster()
.deleteRepository(
Expand Down

0 comments on commit 87ecde6

Please sign in to comment.