Skip to content

Commit

Permalink
Main response should not have status 503 when okay (#29045)
Browse files Browse the repository at this point in the history
The REST status 503 means "I can not handle the request that you sent
me." However today we respond to a main request with a 503 when there
are certain cluster blocks despite still responding with an actual main
response. This is broken, we should respond with a 200 status. This
commit removes this silliness.
  • Loading branch information
jasontedor committed Mar 14, 2018
1 parent 647d0a1 commit 24d10ad
Show file tree
Hide file tree
Showing 9 changed files with 24 additions and 45 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,6 @@
import org.elasticsearch.action.main.MainRequest;
import org.elasticsearch.action.main.MainResponse;
import org.elasticsearch.action.support.PlainActionFuture;
import org.elasticsearch.client.Request;
import org.elasticsearch.client.Response;
import org.elasticsearch.client.ResponseListener;
import org.elasticsearch.client.RestClient;
import org.elasticsearch.client.RestHighLevelClient;
import org.elasticsearch.cluster.ClusterName;
import org.elasticsearch.common.SuppressForbidden;
import org.elasticsearch.common.xcontent.XContentHelper;
Expand Down Expand Up @@ -162,7 +157,7 @@ private Response mockPerformRequest(Header httpHeader) throws IOException {
ProtocolVersion protocol = new ProtocolVersion("HTTP", 1, 1);
when(mockResponse.getStatusLine()).thenReturn(new BasicStatusLine(protocol, 200, "OK"));

MainResponse response = new MainResponse(httpHeader.getValue(), Version.CURRENT, ClusterName.DEFAULT, "_na", Build.CURRENT, true);
MainResponse response = new MainResponse(httpHeader.getValue(), Version.CURRENT, ClusterName.DEFAULT, "_na", Build.CURRENT);
BytesRef bytesRef = XContentHelper.toXContent(response, XContentType.JSON, false).toBytesRef();
when(mockResponse.getEntity()).thenReturn(new ByteArrayEntity(bytesRef.bytes, ContentType.APPLICATION_JSON));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ public void testPingSocketTimeout() throws IOException {
public void testInfo() throws IOException {
Header[] headers = randomHeaders(random(), "Header");
MainResponse testInfo = new MainResponse("nodeName", Version.CURRENT, new ClusterName("clusterName"), "clusterUuid",
Build.CURRENT, true);
Build.CURRENT);
mockResponse(testInfo);
MainResponse receivedInfo = restHighLevelClient.info(headers);
assertEquals(testInfo, receivedInfo);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ public void testMain() throws IOException {
//tag::main-execute
MainResponse response = client.info();
//end::main-execute
assertTrue(response.isAvailable());
//tag::main-response
ClusterName clusterName = response.getClusterName(); // <1>
String clusterUuid = response.getClusterUuid(); // <2>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,18 +41,16 @@ public class MainResponse extends ActionResponse implements ToXContentObject {
private ClusterName clusterName;
private String clusterUuid;
private Build build;
boolean available;

MainResponse() {
}

public MainResponse(String nodeName, Version version, ClusterName clusterName, String clusterUuid, Build build, boolean available) {
public MainResponse(String nodeName, Version version, ClusterName clusterName, String clusterUuid, Build build) {
this.nodeName = nodeName;
this.version = version;
this.clusterName = clusterName;
this.clusterUuid = clusterUuid;
this.build = build;
this.available = available;
}

public String getNodeName() {
Expand All @@ -75,10 +73,6 @@ public Build getBuild() {
return build;
}

public boolean isAvailable() {
return available;
}

@Override
public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
Expand All @@ -87,7 +81,9 @@ public void writeTo(StreamOutput out) throws IOException {
clusterName.writeTo(out);
out.writeString(clusterUuid);
Build.writeBuild(build, out);
out.writeBoolean(available);
if (out.getVersion().before(Version.V_7_0_0_alpha1)) {
out.writeBoolean(true);
}
}

@Override
Expand All @@ -98,7 +94,9 @@ public void readFrom(StreamInput in) throws IOException {
clusterName = new ClusterName(in);
clusterUuid = in.readString();
build = Build.readBuild(in);
available = in.readBoolean();
if (in.getVersion().before(Version.V_7_0_0_alpha1)) {
in.readBoolean();
}
}

@Override
Expand Down Expand Up @@ -133,7 +131,6 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
response.build = new Build((String) value.get("build_hash"), (String) value.get("build_date"),
(boolean) value.get("build_snapshot"));
response.version = Version.fromString((String) value.get("number"));
response.available = true;
}, (parser, context) -> parser.map(), new ParseField("version"));
}

Expand All @@ -154,12 +151,11 @@ public boolean equals(Object o) {
Objects.equals(version, other.version) &&
Objects.equals(clusterUuid, other.clusterUuid) &&
Objects.equals(build, other.build) &&
Objects.equals(available, other.available) &&
Objects.equals(clusterName, other.clusterName);
}

@Override
public int hashCode() {
return Objects.hash(nodeName, version, clusterUuid, build, clusterName, available);
return Objects.hash(nodeName, version, clusterUuid, build, clusterName);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,6 @@ protected void doExecute(MainRequest request, ActionListener<MainResponse> liste
final boolean available = clusterState.getBlocks().hasGlobalBlock(RestStatus.SERVICE_UNAVAILABLE) == false;
listener.onResponse(
new MainResponse(Node.NODE_NAME_SETTING.get(settings), Version.CURRENT, clusterState.getClusterName(),
clusterState.metaData().clusterUUID(), Build.CURRENT, available));
clusterState.metaData().clusterUUID(), Build.CURRENT));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,11 @@ public RestResponse buildResponse(MainResponse mainResponse, XContentBuilder bui
}

static BytesRestResponse convertMainResponse(MainResponse response, RestRequest request, XContentBuilder builder) throws IOException {
RestStatus status = response.isAvailable() ? RestStatus.OK : RestStatus.SERVICE_UNAVAILABLE;

// Default to pretty printing, but allow ?pretty=false to disable
if (request.hasParam("pretty") == false) {
builder.prettyPrint().lfAtEnd();
}
response.toXContent(builder, request);
return new BytesRestResponse(status, builder);
return new BytesRestResponse(RestStatus.OK, builder);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,8 @@ public void testMainActionClusterAvailable() {
final ClusterService clusterService = mock(ClusterService.class);
final ClusterName clusterName = new ClusterName("elasticsearch");
final Settings settings = Settings.builder().put("node.name", "my-node").build();
final boolean available = randomBoolean();
ClusterBlocks blocks;
if (available) {
if (randomBoolean()) {
if (randomBoolean()) {
blocks = ClusterBlocks.EMPTY_CLUSTER_BLOCK;
} else {
Expand Down Expand Up @@ -86,7 +85,6 @@ public void onFailure(Exception e) {
});

assertNotNull(responseRef.get());
assertEquals(available, responseRef.get().isAvailable());
verify(clusterService, times(1)).state();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ protected MainResponse createTestInstance() {
String nodeName = randomAlphaOfLength(10);
Build build = new Build(randomAlphaOfLength(8), new Date(randomNonNegativeLong()).toString(), randomBoolean());
Version version = VersionUtils.randomVersion(random());
return new MainResponse(nodeName, version, clusterName, clusterUuid , build, true);
return new MainResponse(nodeName, version, clusterName, clusterUuid , build);
}

@Override
Expand All @@ -58,7 +58,7 @@ public void testToXContent() throws IOException {
String clusterUUID = randomAlphaOfLengthBetween(10, 20);
Build build = new Build(Build.CURRENT.shortHash(), Build.CURRENT.date(), Build.CURRENT.isSnapshot());
Version version = Version.CURRENT;
MainResponse response = new MainResponse("nodeName", version, new ClusterName("clusterName"), clusterUUID, build, true);
MainResponse response = new MainResponse("nodeName", version, new ClusterName("clusterName"), clusterUUID, build);
XContentBuilder builder = XContentFactory.jsonBuilder();
response.toXContent(builder, ToXContent.EMPTY_PARAMS);
assertEquals("{"
Expand All @@ -80,32 +80,28 @@ public void testToXContent() throws IOException {
@Override
protected MainResponse mutateInstance(MainResponse mutateInstance) {
String clusterUuid = mutateInstance.getClusterUuid();
boolean available = mutateInstance.isAvailable();
Build build = mutateInstance.getBuild();
Version version = mutateInstance.getVersion();
String nodeName = mutateInstance.getNodeName();
ClusterName clusterName = mutateInstance.getClusterName();
switch (randomIntBetween(0, 5)) {
switch (randomIntBetween(0, 4)) {
case 0:
clusterUuid = clusterUuid + randomAlphaOfLength(5);
break;
case 1:
nodeName = nodeName + randomAlphaOfLength(5);
break;
case 2:
available = !available;
break;
case 3:
// toggle the snapshot flag of the original Build parameter
build = new Build(build.shortHash(), build.date(), !build.isSnapshot());
break;
case 4:
case 3:
version = randomValueOtherThan(version, () -> VersionUtils.randomVersion(random()));
break;
case 5:
case 4:
clusterName = new ClusterName(clusterName + randomAlphaOfLength(5));
break;
}
return new MainResponse(nodeName, version, clusterName, clusterUuid, build, available);
return new MainResponse(nodeName, version, clusterName, clusterUuid, build);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import java.util.HashMap;
import java.util.Map;

import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.greaterThan;

public class RestMainActionTests extends ESTestCase {
Expand All @@ -44,12 +45,10 @@ public void testHeadResponse() throws Exception {
final String nodeName = "node1";
final ClusterName clusterName = new ClusterName("cluster1");
final String clusterUUID = randomAlphaOfLengthBetween(10, 20);
final boolean available = randomBoolean();
final RestStatus expectedStatus = available ? RestStatus.OK : RestStatus.SERVICE_UNAVAILABLE;
final Version version = Version.CURRENT;
final Build build = Build.CURRENT;

final MainResponse mainResponse = new MainResponse(nodeName, version, clusterName, clusterUUID, build, available);
final MainResponse mainResponse = new MainResponse(nodeName, version, clusterName, clusterUUID, build);
XContentBuilder builder = JsonXContent.contentBuilder();
RestRequest restRequest = new FakeRestRequest() {
@Override
Expand All @@ -60,7 +59,7 @@ public Method method() {

BytesRestResponse response = RestMainAction.convertMainResponse(mainResponse, restRequest, builder);
assertNotNull(response);
assertEquals(expectedStatus, response.status());
assertThat(response.status(), equalTo(RestStatus.OK));

// the empty responses are handled in the HTTP layer so we do
// not assert on them here
Expand All @@ -70,13 +69,11 @@ public void testGetResponse() throws Exception {
final String nodeName = "node1";
final ClusterName clusterName = new ClusterName("cluster1");
final String clusterUUID = randomAlphaOfLengthBetween(10, 20);
final boolean available = randomBoolean();
final RestStatus expectedStatus = available ? RestStatus.OK : RestStatus.SERVICE_UNAVAILABLE;
final Version version = Version.CURRENT;
final Build build = Build.CURRENT;
final boolean prettyPrint = randomBoolean();

final MainResponse mainResponse = new MainResponse(nodeName, version, clusterName, clusterUUID, build, available);
final MainResponse mainResponse = new MainResponse(nodeName, version, clusterName, clusterUUID, build);
XContentBuilder builder = JsonXContent.contentBuilder();

Map<String, String> params = new HashMap<>();
Expand All @@ -87,7 +84,7 @@ public void testGetResponse() throws Exception {

BytesRestResponse response = RestMainAction.convertMainResponse(mainResponse, restRequest, builder);
assertNotNull(response);
assertEquals(expectedStatus, response.status());
assertThat(response.status(), equalTo(RestStatus.OK));
assertThat(response.content().length(), greaterThan(0));

XContentBuilder responseBuilder = JsonXContent.contentBuilder();
Expand Down

0 comments on commit 24d10ad

Please sign in to comment.