[8.0] Revert "Return 200 OK response code for a cluster health timeout (

#78968)" (#80826) * [8.0] Revert "Return 200 OK response code for a cluster health timeout (#78968)" This reverts commit a2c3dae * Revert "Allow deprecation warning for the return_200_for_cluster_health_timeout parameter (#80178) (#80444)" This reverts commit 4102cf7. * Revert "Drop pre-7.2.0 wire format in ClusterHealthRequest (#79551)" This reverts commit b9fbe66. * Revert "Adjust the BWC version for the return200ForClusterHealthTimeout field (#79436)" This reverts commit f60bda5. * Revert "Use query param instead of a system property for opting in for new cluster health response code (#79351)" This reverts commit 8901a99 * Revert "Deprecate returning 408 for a server timeout on `_cluster/health` (#78180)" This reverts commit f266eb3 * Drop pre-7.2.0 wire format in ClusterHealthRequest (#79551) This reverts commit fa4d562. * Revert "[8.0] Disable BWC for #80821 (#80840)"
elastic · Nov 18, 2021 · d8b5bfe · d8b5bfe
1 parent 43f61bb
commit d8b5bfe
Show file tree

Hide file tree

Showing 20 changed files with 30 additions and 185 deletions.
diff --git a/build.gradle b/build.gradle
@@ -125,9 +125,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

diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/ClusterClientIT.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/ClusterClientIT.java
@@ -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);
     }

diff --git a/docs/changelog/78968.yaml b/docs/changelog/78968.yaml
diff --git a/docs/reference/cluster/health.asciidoc b/docs/reference/cluster/health.asciidoc
@@ -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}
 

diff --git a/rest-api-spec/src/main/resources/rest-api-spec/api/cluster.health.json b/rest-api-spec/src/main/resources/rest-api-spec/api/cluster.health.json
@@ -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"
       }
     }
   }

diff --git a/...-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.health/20_request_timeout.yml b/...-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.health/20_request_timeout.yml
@@ -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
@@ -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
@@ -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 }
diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/health/ClusterHealthRequest.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/health/ClusterHealthRequest.java
@@ -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'.
@@ -63,7 +61,6 @@ public ClusterHealthRequest(StreamInput in) throws IOException {
         }
         waitForNoInitializingShards = in.readBoolean();
         indicesOptions = IndicesOptions.readIndicesOptions(in);
-        return200ForClusterHealthTimeout = in.readBoolean();
     }
 
     @Override
@@ -92,7 +89,6 @@ public void writeTo(StreamOutput out) throws IOException {
         }
         out.writeBoolean(waitForNoInitializingShards);
         indicesOptions.writeIndicesOptions(out);
-        out.writeBoolean(return200ForClusterHealthTimeout);
     }
 
     @Override
@@ -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.

diff --git a/...er/src/main/java/org/elasticsearch/action/admin/cluster/health/ClusterHealthResponse.java b/...er/src/main/java/org/elasticsearch/action/admin/cluster/health/ClusterHealthResponse.java
@@ -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;
@@ -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);
@@ -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(
@@ -184,8 +178,7 @@ public ClusterHealthResponse(
         int numberOfPendingTasks,
         int numberOfInFlightFetch,
         int delayedUnassignedShards,
-        TimeValue taskMaxWaitingTime,
-        boolean return200ForServerTimeout
+        TimeValue taskMaxWaitingTime
     ) {
         this.clusterName = clusterName;
         this.numberOfPendingTasks = numberOfPendingTasks;
@@ -194,7 +187,6 @@ public ClusterHealthResponse(
         this.taskMaxWaitingTime = taskMaxWaitingTime;
         this.clusterStateHealth = new ClusterStateHealth(clusterState, concreteIndices);
         this.clusterHealthStatus = clusterStateHealth.getStatus();
-        this.return200ForClusterHealthTimeout = return200ForServerTimeout;
     }
 
     /**
@@ -333,7 +325,6 @@ public void writeTo(StreamOutput out) throws IOException {
         out.writeInt(numberOfInFlightFetch);
         out.writeInt(delayedUnassignedShards);
         out.writeTimeValue(taskMaxWaitingTime);
-        out.writeBoolean(return200ForClusterHealthTimeout);
     }
 
     @Override
@@ -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

diff --git a/...main/java/org/elasticsearch/action/admin/cluster/health/TransportClusterHealthAction.java b/...main/java/org/elasticsearch/action/admin/cluster/health/TransportClusterHealthAction.java
@@ -411,8 +411,7 @@ private ClusterHealthResponse clusterHealth(
                 numberOfPendingTasks,
                 numberOfInFlightFetch,
                 UnassignedInfo.getNumberOfDelayedUnassigned(clusterState),
-                pendingTaskTimeInQueue,
-                request.doesReturn200ForClusterHealthTimeout()
+                pendingTaskTimeInQueue
             );
             response.setStatus(ClusterHealthStatus.RED);
             return response;
@@ -425,8 +424,7 @@ private ClusterHealthResponse clusterHealth(
             numberOfPendingTasks,
             numberOfInFlightFetch,
             UnassignedInfo.getNumberOfDelayedUnassigned(clusterState),
-            pendingTaskTimeInQueue,
-            request.doesReturn200ForClusterHealthTimeout()
+            pendingTaskTimeInQueue
         );
     }
 }
diff --git a/server/src/main/java/org/elasticsearch/common/xcontent/StatusToXContentObject.java b/server/src/main/java/org/elasticsearch/common/xcontent/StatusToXContentObject.java
@@ -7,7 +7,6 @@
  */
 package org.elasticsearch.common.xcontent;
 
-import org.elasticsearch.core.RestApiVersion;
 import org.elasticsearch.rest.RestStatus;
 import org.elasticsearch.xcontent.ToXContentObject;
 
@@ -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();
-    }
 }
diff --git a/server/src/main/java/org/elasticsearch/rest/action/RestStatusToXContentListener.java b/server/src/main/java/org/elasticsearch/rest/action/RestStatusToXContentListener.java
@@ -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) {

diff --git a/...er/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestClusterHealthAction.java b/...er/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestClusterHealthAction.java
@@ -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;
@@ -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}"));
@@ -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;
     }
 

diff --git a/...rc/test/java/org/elasticsearch/action/admin/cluster/health/ClusterHealthRequestTests.java b/...rc/test/java/org/elasticsearch/action/admin/cluster/health/ClusterHealthRequestTests.java
@@ -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() {