Return 408 REQUEST_TIMEOUT if _cluster/health times out

Today we return `200 OK` which is misleading since we really didn't produce a valid response / didn't wait long enough.
elastic · Aug 12, 2015 · fe3179d · fe3179d
1 parent 777921d
commit fe3179d
Show file tree

Hide file tree

Showing 6 changed files with 46 additions and 12 deletions.
diff --git a/core/src/main/java/org/elasticsearch/action/admin/cluster/health/ClusterHealthResponse.java b/core/src/main/java/org/elasticsearch/action/admin/cluster/health/ClusterHealthResponse.java
@@ -31,10 +31,7 @@
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.unit.TimeValue;
-import org.elasticsearch.common.xcontent.ToXContent;
-import org.elasticsearch.common.xcontent.XContentBuilder;
-import org.elasticsearch.common.xcontent.XContentBuilderString;
-import org.elasticsearch.common.xcontent.XContentFactory;
+import org.elasticsearch.common.xcontent.*;
 import org.elasticsearch.rest.RestStatus;
 
 import java.io.IOException;
@@ -49,7 +46,7 @@
 /**
  *
  */
-public class ClusterHealthResponse extends ActionResponse implements Iterable<ClusterIndexHealth>, ToXContent {
+public class ClusterHealthResponse extends ActionResponse implements Iterable<ClusterIndexHealth>, StatusToXContent {
 
     private String clusterName;
     int numberOfNodes = 0;
@@ -332,6 +329,11 @@ public String toString() {
         }
     }
 
+    @Override
+    public RestStatus status() {
+        return isTimedOut() ? RestStatus.REQUEST_TIMEOUT : RestStatus.OK;
+    }
+
     static final class Fields {
         static final XContentBuilderString CLUSTER_NAME = new XContentBuilderString("cluster_name");
         static final XContentBuilderString STATUS = new XContentBuilderString("status");

diff --git a/...main/java/org/elasticsearch/rest/action/admin/cluster/health/RestClusterHealthAction.java b/...main/java/org/elasticsearch/rest/action/admin/cluster/health/RestClusterHealthAction.java
@@ -27,7 +27,7 @@
 import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.rest.*;
-import org.elasticsearch.rest.action.support.RestToXContentListener;
+import org.elasticsearch.rest.action.support.RestStatusToXContentListener;
 
 import java.util.Locale;
 
@@ -59,7 +59,6 @@ public void handleRequest(final RestRequest request, final RestChannel channel,
         clusterHealthRequest.waitForRelocatingShards(request.paramAsInt("wait_for_relocating_shards", clusterHealthRequest.waitForRelocatingShards()));
         clusterHealthRequest.waitForActiveShards(request.paramAsInt("wait_for_active_shards", clusterHealthRequest.waitForActiveShards()));
         clusterHealthRequest.waitForNodes(request.param("wait_for_nodes", clusterHealthRequest.waitForNodes()));
-
-        client.admin().cluster().health(clusterHealthRequest, new RestToXContentListener<ClusterHealthResponse>(channel));
+        client.admin().cluster().health(clusterHealthRequest, new RestStatusToXContentListener<ClusterHealthResponse>(channel));
     }
 }
diff --git a/.../cluster/ClusterHealthResponsesTests.java → ...r/health/ClusterHealthResponsesTests.java b/.../cluster/ClusterHealthResponsesTests.java → ...r/health/ClusterHealthResponsesTests.java
@@ -17,7 +17,7 @@
  * under the License.
  */
 
-package org.elasticsearch.cluster;
+package org.elasticsearch.action.admin.cluster.health;
 
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.Version;
@@ -26,6 +26,8 @@
 import org.elasticsearch.action.admin.cluster.health.ClusterIndexHealth;
 import org.elasticsearch.action.admin.cluster.health.ClusterShardHealth;
 import org.elasticsearch.action.support.IndicesOptions;
+import org.elasticsearch.cluster.ClusterName;
+import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.metadata.IndexMetaData;
 import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
 import org.elasticsearch.cluster.metadata.MetaData;
@@ -35,6 +37,7 @@
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.index.shard.ShardId;
+import org.elasticsearch.rest.RestStatus;
 import org.elasticsearch.test.ESTestCase;
 import org.hamcrest.Matchers;
 import org.junit.Test;
@@ -181,6 +184,18 @@ private void assertClusterHealth(ClusterHealthResponse clusterHealth, ShardCount
         assertThat(clusterHealth.getValidationFailures(), empty());
     }
 
+    public void testIsTimeout() throws IOException {
+        ClusterHealthResponse res = new ClusterHealthResponse();
+        for (int i = 0; i < 5; i++) {
+            res.timedOut = randomBoolean();
+            if (res.isTimedOut()) {
+                assertEquals(RestStatus.REQUEST_TIMEOUT, res.status());
+            } else {
+                assertEquals(RestStatus.OK, res.status());
+            }
+        }
+    }
+
     @Test
     public void testClusterHealth() throws IOException {
         ShardCounter counter = new ShardCounter();

diff --git a/core/src/test/java/org/elasticsearch/test/rest/section/DoSection.java b/core/src/test/java/org/elasticsearch/test/rest/section/DoSection.java
@@ -131,6 +131,7 @@ private String formatStatusCodeMessage(RestResponse restResponse, String expecte
         catches.put("missing", tuple("404", equalTo(404)));
         catches.put("conflict", tuple("409", equalTo(409)));
         catches.put("forbidden", tuple("403", equalTo(403)));
-        catches.put("request", tuple("4xx|5xx", allOf(greaterThanOrEqualTo(400), not(equalTo(404)), not(equalTo(409)), not(equalTo(403)))));
+        catches.put("request_timeout", tuple("408", equalTo(408)));
+        catches.put("request", tuple("4xx|5xx", allOf(greaterThanOrEqualTo(400), not(equalTo(404)), not(equalTo(408)), not(equalTo(409)), not(equalTo(403)))));
     }
 }
diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/cat.allocation/10_basic.yaml b/rest-api-spec/src/main/resources/rest-api-spec/test/cat.allocation/10_basic.yaml
@@ -48,8 +48,7 @@
 
   - do:
         cluster.health:
-            wait_for_status: green
-            timeout: 1s
+            wait_for_status: yellow
 
   - do:
         cat.allocation:

diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.health/20_request_timeout.yaml b/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.health/20_request_timeout.yaml
@@ -0,0 +1,18 @@
+---
+"cluster health request timeout":
+  - do:
+      catch: request_timeout
+      cluster.health:
+        wait_for_nodes: 10
+        timeout: 1s
+
+  - 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 }