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.

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

Revert "Return 200 OK response code for a cluster health timeout (#78968)" #80821

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,9 @@ tasks.register("verifyVersions") {
* after the backport of the backcompat code is complete.
*/

boolean bwc_tests_enabled = false
boolean bwc_tests_enabled = true
// place a PR link here when committing bwc changes:
String bwc_tests_disabled_issue = "https://github.com/elastic/elasticsearch/pull/80821"
String bwc_tests_disabled_issue = ""
/*
* FIPS 140-2 behavior was fixed in 7.11.0. Before that there is no way to run elasticsearch in a
* JVM that is properly configured to be in fips mode with BCFIPS. For now we need to disable
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ public void testClusterHealthNotFoundIndex() throws IOException {

assertThat(response, notNullValue());
assertThat(response.isTimedOut(), equalTo(true));
assertThat(response.status(), equalTo(RestStatus.OK));
assertThat(response.status(), equalTo(RestStatus.REQUEST_TIMEOUT));
assertThat(response.getStatus(), equalTo(ClusterHealthStatus.RED));
assertNoIndices(response);
}
Expand Down
20 changes: 0 additions & 20 deletions docs/changelog/78968.yaml

This file was deleted.

5 changes: 0 additions & 5 deletions docs/reference/cluster/health.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,6 @@ include::{es-repo-dir}/rest-api/common-parms.asciidoc[tag=timeoutparms]
provided or better, i.e. `green` > `yellow` > `red`. By default, will not
wait for any status.

`return_200_for_cluster_health_timeout`::
(Optional, Boolean) A boolean value which controls whether to return HTTP 200
status code instead of HTTP 408 in case of a cluster health timeout from
the server side. Defaults to false.

[[cluster-health-api-response-body]]
==== {api-response-body-title}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,6 @@
"red"
],
"description":"Wait until cluster is in a specific state"
},
"return_200_for_cluster_health_timeout":{
"type":"boolean",
"description":"Whether to return HTTP 200 instead of 408 in case of a cluster health timeout from the server side"
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
---
"cluster health request timeout on waiting for nodes":
- skip:
version: " - 8.0.99"
reason: "Set for 7.99.99 when back-ported to 8.0"
- do:
catch: request_timeout
cluster.health:
wait_for_nodes: 10
timeout: 1ms
Expand All @@ -21,10 +19,8 @@

---
"cluster health request timeout waiting for active shards":
- skip:
version: " - 8.0.99"
reason: "Set for 7.99.99 when back-ported to 8.0"
- do:
catch: request_timeout
cluster.health:
timeout: 1ms
wait_for_active_shards: 5
Expand All @@ -39,28 +35,3 @@
- match: { initializing_shards: 0 }
- match: { unassigned_shards: 0 }
- gte: { number_of_pending_tasks: 0 }

---
"cluster health request timeout with 200 response code":
- skip:
version: " - 7.15.99"
reason: "return_200_for_cluster_health_timeout was added in 7.16"
features: [ "allowed_warnings" ]
- do:
allowed_warnings:
- 'the [return_200_for_cluster_health_timeout] parameter is deprecated and will be removed in a future release.'
cluster.health:
timeout: 1ms
wait_for_active_shards: 5
return_200_for_cluster_health_timeout: true

- is_true: cluster_name
- is_true: timed_out
- gte: { number_of_nodes: 1 }
- gte: { number_of_data_nodes: 1 }
- match: { active_primary_shards: 0 }
- match: { active_shards: 0 }
- match: { relocating_shards: 0 }
- match: { initializing_shards: 0 }
- match: { unassigned_shards: 0 }
- gte: { number_of_pending_tasks: 0 }
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ public class ClusterHealthRequest extends MasterNodeReadRequest<ClusterHealthReq
private ActiveShardCount waitForActiveShards = ActiveShardCount.NONE;
private String waitForNodes = "";
private Priority waitForEvents = null;
private boolean return200ForClusterHealthTimeout;

/**
* Only used by the high-level REST Client. Controls the details level of the health information returned.
* The default value is 'cluster'.
Expand Down Expand Up @@ -63,7 +61,6 @@ public ClusterHealthRequest(StreamInput in) throws IOException {
}
waitForNoInitializingShards = in.readBoolean();
indicesOptions = IndicesOptions.readIndicesOptions(in);
return200ForClusterHealthTimeout = in.readBoolean();
}

@Override
Expand Down Expand Up @@ -92,7 +89,6 @@ public void writeTo(StreamOutput out) throws IOException {
}
out.writeBoolean(waitForNoInitializingShards);
indicesOptions.writeIndicesOptions(out);
out.writeBoolean(return200ForClusterHealthTimeout);
}

@Override
Expand Down Expand Up @@ -236,18 +232,6 @@ public Priority waitForEvents() {
return this.waitForEvents;
}

public boolean doesReturn200ForClusterHealthTimeout() {
return return200ForClusterHealthTimeout;
}

/**
* Sets whether to return HTTP 200 status code instead of HTTP 408 in case of a
* cluster health timeout from the server side.
*/
public void return200ForClusterHealthTimeout(boolean return200ForClusterHealthTimeout) {
this.return200ForClusterHealthTimeout = return200ForClusterHealthTimeout;
}

/**
* Set the level of detail for the health information to be returned.
* Only used by the high-level REST Client.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.StatusToXContentObject;
import org.elasticsearch.core.RestApiVersion;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.xcontent.ConstructingObjectParser;
Expand Down Expand Up @@ -152,7 +151,8 @@ public class ClusterHealthResponse extends ActionResponse implements StatusToXCo
private boolean timedOut = false;
private ClusterStateHealth clusterStateHealth;
private ClusterHealthStatus clusterHealthStatus;
private boolean return200ForClusterHealthTimeout;

public ClusterHealthResponse() {}

public ClusterHealthResponse(StreamInput in) throws IOException {
super(in);
Expand All @@ -164,17 +164,11 @@ public ClusterHealthResponse(StreamInput in) throws IOException {
numberOfInFlightFetch = in.readInt();
delayedUnassignedShards = in.readInt();
taskMaxWaitingTime = in.readTimeValue();
return200ForClusterHealthTimeout = in.readBoolean();
}

/** needed for plugins BWC */
public ClusterHealthResponse(
String clusterName,
String[] concreteIndices,
ClusterState clusterState,
boolean return200ForServerTimeout
) {
this(clusterName, concreteIndices, clusterState, -1, -1, -1, TimeValue.timeValueHours(0), return200ForServerTimeout);
public ClusterHealthResponse(String clusterName, String[] concreteIndices, ClusterState clusterState) {
this(clusterName, concreteIndices, clusterState, -1, -1, -1, TimeValue.timeValueHours(0));
}

public ClusterHealthResponse(
Expand All @@ -184,8 +178,7 @@ public ClusterHealthResponse(
int numberOfPendingTasks,
int numberOfInFlightFetch,
int delayedUnassignedShards,
TimeValue taskMaxWaitingTime,
boolean return200ForServerTimeout
TimeValue taskMaxWaitingTime
) {
this.clusterName = clusterName;
this.numberOfPendingTasks = numberOfPendingTasks;
Expand All @@ -194,7 +187,6 @@ public ClusterHealthResponse(
this.taskMaxWaitingTime = taskMaxWaitingTime;
this.clusterStateHealth = new ClusterStateHealth(clusterState, concreteIndices);
this.clusterHealthStatus = clusterStateHealth.getStatus();
this.return200ForClusterHealthTimeout = return200ForServerTimeout;
}

/**
Expand Down Expand Up @@ -333,7 +325,6 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeInt(numberOfInFlightFetch);
out.writeInt(delayedUnassignedShards);
out.writeTimeValue(taskMaxWaitingTime);
out.writeBoolean(return200ForClusterHealthTimeout);
}

@Override
Expand All @@ -343,16 +334,7 @@ public String toString() {

@Override
public RestStatus status() {
return status(RestApiVersion.current());
}

@Override
public RestStatus status(RestApiVersion restApiVersion) {
// Legacy behaviour
if (isTimedOut() && restApiVersion == RestApiVersion.V_7 && return200ForClusterHealthTimeout == false) {
return RestStatus.REQUEST_TIMEOUT;
}
return RestStatus.OK;
return isTimedOut() ? RestStatus.REQUEST_TIMEOUT : RestStatus.OK;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -411,8 +411,7 @@ private ClusterHealthResponse clusterHealth(
numberOfPendingTasks,
numberOfInFlightFetch,
UnassignedInfo.getNumberOfDelayedUnassigned(clusterState),
pendingTaskTimeInQueue,
request.doesReturn200ForClusterHealthTimeout()
pendingTaskTimeInQueue
);
response.setStatus(ClusterHealthStatus.RED);
return response;
Expand All @@ -425,8 +424,7 @@ private ClusterHealthResponse clusterHealth(
numberOfPendingTasks,
numberOfInFlightFetch,
UnassignedInfo.getNumberOfDelayedUnassigned(clusterState),
pendingTaskTimeInQueue,
request.doesReturn200ForClusterHealthTimeout()
pendingTaskTimeInQueue
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
*/
package org.elasticsearch.common.xcontent;

import org.elasticsearch.core.RestApiVersion;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.xcontent.ToXContentObject;

Expand All @@ -21,8 +20,4 @@ public interface StatusToXContentObject extends ToXContentObject {
* Returns the REST status to make sure it is returned correctly
*/
RestStatus status();

default RestStatus status(RestApiVersion restApiVersion) {
return status();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public RestStatusToXContentListener(RestChannel channel, Function<Response, Stri
public RestResponse buildResponse(Response response, XContentBuilder builder) throws Exception {
assert response.isFragment() == false; // would be nice if we could make default methods final
response.toXContent(builder, channel.request());
RestResponse restResponse = new BytesRestResponse(response.status(builder.getRestApiVersion()), builder);
RestResponse restResponse = new BytesRestResponse(response.status(), builder);
if (RestStatus.CREATED == restResponse.status()) {
final String location = extractLocation.apply(response);
if (location != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,9 @@
import org.elasticsearch.cluster.health.ClusterHealthStatus;
import org.elasticsearch.common.Priority;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.logging.DeprecationCategory;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.rest.BaseRestHandler;
import org.elasticsearch.rest.RestRequest;
import org.elasticsearch.rest.action.RestStatusToXContentListener;
import org.elasticsearch.rest.action.search.RestSearchAction;

import java.io.IOException;
import java.util.Collections;
Expand All @@ -33,12 +30,6 @@

public class RestClusterHealthAction extends BaseRestHandler {

private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestSearchAction.class);
private static final String RETURN_200_FOR_CLUSTER_HEALTH_TIMEOUT = "return_200_for_cluster_health_timeout";
private static final String CLUSTER_HEALTH_REQUEST_TIMEOUT_DEPRECATION_MSG = "the ["
+ RETURN_200_FOR_CLUSTER_HEALTH_TIMEOUT
+ "] parameter is deprecated and will be removed in a future release.";

@Override
public List<Route> routes() {
return List.of(new Route(GET, "/_cluster/health"), new Route(GET, "/_cluster/health/{index}"));
Expand Down Expand Up @@ -90,15 +81,6 @@ public static ClusterHealthRequest fromRequest(final RestRequest request) {
if (request.param("wait_for_events") != null) {
clusterHealthRequest.waitForEvents(Priority.valueOf(request.param("wait_for_events").toUpperCase(Locale.ROOT)));
}
String return200ForClusterHealthTimeout = request.param(RETURN_200_FOR_CLUSTER_HEALTH_TIMEOUT);
if (return200ForClusterHealthTimeout != null) {
deprecationLogger.warn(
DeprecationCategory.API,
"cluster_health_request_timeout",
CLUSTER_HEALTH_REQUEST_TIMEOUT_DEPRECATION_MSG
);
}
clusterHealthRequest.return200ForClusterHealthTimeout(Boolean.parseBoolean(return200ForClusterHealthTimeout));
return clusterHealthRequest;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ public void testSerialize() throws Exception {
assertThat(cloneRequest.waitForEvents(), equalTo(originalRequest.waitForEvents()));
assertIndicesEquals(cloneRequest.indices(), originalRequest.indices());
assertThat(cloneRequest.indicesOptions(), equalTo(originalRequest.indicesOptions()));
assertThat(cloneRequest.doesReturn200ForClusterHealthTimeout(), equalTo(originalRequest.doesReturn200ForClusterHealthTimeout()));
}

public void testRequestReturnsHiddenIndicesByDefault() {
Expand Down
Loading