honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Prevent cause from being null in ShardOperationFailedException (#32640) 

`ShardOperationFailedException` and corresponding implementors seem to suggest that the cause may be null, case that is also handled in a few places. Yet, it does not seem to be possible in practice for the cause to be null, hence we can clean that up and enforce the cause to be a non null value. This is best done by making `ShardOperationFailedException` an abstract class rather than an interface, which holds the basic member instance that all the subclasses have in common and can also enforce that cause, status and reason are non null.
This commit is contained in:
Luca Cavanna 2018-08-08 09:59:22 +02:00 committed by GitHub
parent 5c2ef5e869
commit 3e437438d5
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
11 changed files with 151 additions and 305 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
server/src/main/java/org/elasticsearch/ExceptionsHelper.java
View File

@ -308,13 +308,8 @@ public final class ExceptionsHelper {
}
}
this.index = indexName;
if (cause == null) {
this.reason = failure.reason();
this.causeType = null;
} else {
this.reason = cause.getMessage();
this.causeType = cause.getClass();
}
this.reason = cause.getMessage();
this.causeType = cause.getClass();
}
@Override

45
server/src/main/java/org/elasticsearch/action/ShardOperationFailedException.java
View File

@ -19,39 +19,70 @@
package org.elasticsearch.action;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.io.stream.Streamable;
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.rest.RestStatus;
import java.util.Objects;
/**
* An exception indicating that a failure occurred performing an operation on the shard.
*
*
*/
public interface ShardOperationFailedException extends Streamable, ToXContent {
public abstract class ShardOperationFailedException implements Streamable, ToXContent {
protected String index;
protected int shardId;
protected String reason;
protected RestStatus status;
protected Throwable cause;
protected ShardOperationFailedException() {
}
protected ShardOperationFailedException(@Nullable String index, int shardId, String reason, RestStatus status, Throwable cause) {
this.index = index;
this.shardId = shardId;
this.reason = Objects.requireNonNull(reason, "reason cannot be null");
this.status = Objects.requireNonNull(status, "status cannot be null");
this.cause = Objects.requireNonNull(cause, "cause cannot be null");
}
/**
* The index the operation failed on. Might return {@code null} if it can't be derived.
*/
String index();
@Nullable
public final String index() {
return index;
}
/**
* The index the operation failed on. Might return {@code -1} if it can't be derived.
*/
int shardId();
public final int shardId() {
return shardId;
}
/**
* The reason of the failure.
*/
String reason();
public final String reason() {
return reason;
}
/**
* The status of the failure.
*/
RestStatus status();
public final RestStatus status() {
return status;
}
/**
* The cause of this failure
*/
Throwable getCause();
public final Throwable getCause() {
return cause;
}
}

3
server/src/main/java/org/elasticsearch/action/admin/indices/shards/IndicesShardStoresResponse.java
View File

@ -21,7 +21,6 @@ package org.elasticsearch.action.admin.indices.shards;
import com.carrotsearch.hppc.cursors.IntObjectCursor;
import com.carrotsearch.hppc.cursors.ObjectObjectCursor;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.Version;
import org.elasticsearch.action.ActionResponse;
@ -248,7 +247,7 @@ public class IndicesShardStoresResponse extends ActionResponse implements ToXCon
return nodeId;
}
public static Failure readFailure(StreamInput in) throws IOException {
static Failure readFailure(StreamInput in) throws IOException {
Failure failure = new Failure();
failure.readFrom(in);
return failure;

3
server/src/main/java/org/elasticsearch/action/search/SearchPhaseExecutionException.java
View File

@ -138,8 +138,7 @@ public class SearchPhaseExecutionException extends ElasticsearchException {
builder.field("grouped", group); // notify that it's grouped
builder.field("failed_shards");
builder.startArray();
ShardOperationFailedException[] failures = params.paramAsBoolean("group_shard_failures", true) ?
ExceptionsHelper.groupBy(shardFailures) : shardFailures;
ShardOperationFailedException[] failures = group ? ExceptionsHelper.groupBy(shardFailures) : shardFailures;
for (ShardOperationFailedException failure : failures) {
builder.startObject();
failure.toXContent(builder, params);

74
server/src/main/java/org/elasticsearch/action/search/ShardSearchFailure.java
View File

@ -43,7 +43,7 @@ import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpect
/**
* Represents a failure to search on a specific shard.
*/
public class ShardSearchFailure implements ShardOperationFailedException {
public class ShardSearchFailure extends ShardOperationFailedException {
private static final String REASON_FIELD = "reason";
private static final String NODE_FIELD = "node";
@ -53,9 +53,6 @@ public class ShardSearchFailure implements ShardOperationFailedException {
public static final ShardSearchFailure[] EMPTY_ARRAY = new ShardSearchFailure[0];
private SearchShardTarget shardTarget;
private String reason;
private RestStatus status;
private Throwable cause;
private ShardSearchFailure() {
@ -66,25 +63,18 @@ public class ShardSearchFailure implements ShardOperationFailedException {
}
public ShardSearchFailure(Exception e, @Nullable SearchShardTarget shardTarget) {
super(shardTarget == null ? null : shardTarget.getFullyQualifiedIndexName(),
shardTarget == null ? -1 : shardTarget.getShardId().getId(),
ExceptionsHelper.detailedMessage(e),
ExceptionsHelper.status(ExceptionsHelper.unwrapCause(e)),
ExceptionsHelper.unwrapCause(e));
final Throwable actual = ExceptionsHelper.unwrapCause(e);
if (actual instanceof SearchException) {
this.shardTarget = ((SearchException) actual).shard();
} else if (shardTarget != null) {
this.shardTarget = shardTarget;
}
status = ExceptionsHelper.status(actual);
this.reason = ExceptionsHelper.detailedMessage(e);
this.cause = actual;
}
public ShardSearchFailure(String reason, SearchShardTarget shardTarget) {
this(reason, shardTarget, RestStatus.INTERNAL_SERVER_ERROR);
}
private ShardSearchFailure(String reason, SearchShardTarget shardTarget, RestStatus status) {
this.shardTarget = shardTarget;
this.reason = reason;
this.status = status;
}
/**
@ -95,41 +85,6 @@ public class ShardSearchFailure implements ShardOperationFailedException {
return this.shardTarget;
}
@Override
public RestStatus status() {
return this.status;
}
/**
* The index the search failed on.
*/
@Override
public String index() {
if (shardTarget != null) {
return shardTarget.getFullyQualifiedIndexName();
}
return null;
}
/**
* The shard id the search failed on.
*/
@Override
public int shardId() {
if (shardTarget != null) {
return shardTarget.getShardId().id();
}
return -1;
}
/**
* The reason of the failure.
*/
@Override
public String reason() {
return this.reason;
}
@Override
public String toString() {
return "shard [" + (shardTarget == null ? "_na" : shardTarget) + "], reason [" + reason + "], cause [" +
@ -172,12 +127,10 @@ public class ShardSearchFailure implements ShardOperationFailedException {
if (shardTarget != null) {
builder.field(NODE_FIELD, shardTarget.getNodeId());
}
if (cause != null) {
builder.field(REASON_FIELD);
builder.startObject();
ElasticsearchException.generateThrowableXContent(builder, params, cause);
builder.endObject();
}
builder.field(REASON_FIELD);
builder.startObject();
ElasticsearchException.generateThrowableXContent(builder, params, cause);
builder.endObject();
return builder;
}
@ -225,9 +178,4 @@ public class ShardSearchFailure implements ShardOperationFailedException {
}
return new ShardSearchFailure(exception, searchShardTarget);
}
@Override
public Throwable getCause() {
return cause;
}
}

69
server/src/main/java/org/elasticsearch/action/support/DefaultShardOperationFailedException.java
View File

@ -28,8 +28,6 @@ import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.ConstructingObjectParser;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.Index;
import org.elasticsearch.index.shard.ShardId;
import org.elasticsearch.rest.RestStatus;
import java.io.IOException;
@ -37,7 +35,7 @@ import java.io.IOException;
import static org.elasticsearch.ExceptionsHelper.detailedMessage;
import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
public class DefaultShardOperationFailedException implements ShardOperationFailedException {
public class DefaultShardOperationFailedException extends ShardOperationFailedException {
private static final String INDEX = "index";
private static final String SHARD_ID = "shard";
@ -52,56 +50,16 @@ public class DefaultShardOperationFailedException implements ShardOperationFaile
PARSER.declareObject(constructorArg(), (p, c) -> ElasticsearchException.fromXContent(p), new ParseField(REASON));
}
private String index;
private int shardId;
private Throwable reason;
private RestStatus status;
protected DefaultShardOperationFailedException() {
}
public DefaultShardOperationFailedException(ElasticsearchException e) {
Index index = e.getIndex();
this.index = index == null ? null : index.getName();
ShardId shardId = e.getShardId();
this.shardId = shardId == null ? -1 : shardId.id();
this.reason = e;
this.status = e.status();
super(e.getIndex() == null ? null : e.getIndex().getName(), e.getShardId() == null ? -1 : e.getShardId().getId(),
detailedMessage(e), e.status(), e);
}
public DefaultShardOperationFailedException(String index, int shardId, Throwable reason) {
this.index = index;
this.shardId = shardId;
this.reason = reason;
this.status = ExceptionsHelper.status(reason);
}
@Override
public String index() {
return this.index;
}
@Override
public int shardId() {
return this.shardId;
}
@Override
public String reason() {
return detailedMessage(reason);
}
@Override
public RestStatus status() {
return status;
}
@Override
public Throwable getCause() {
return reason;
public DefaultShardOperationFailedException(String index, int shardId, Throwable cause) {
super(index, shardId, detailedMessage(cause), ExceptionsHelper.status(cause), cause);
}
public static DefaultShardOperationFailedException readShardOperationFailed(StreamInput in) throws IOException {
@ -112,24 +70,17 @@ public class DefaultShardOperationFailedException implements ShardOperationFaile
@Override
public void readFrom(StreamInput in) throws IOException {
if (in.readBoolean()) {
index = in.readString();
}
index = in.readOptionalString();
shardId = in.readVInt();
reason = in.readException();
cause = in.readException();
status = RestStatus.readFrom(in);
}
@Override
public void writeTo(StreamOutput out) throws IOException {
if (index == null) {
out.writeBoolean(false);
} else {
out.writeBoolean(true);
out.writeString(index);
}
out.writeOptionalString(index);
out.writeVInt(shardId);
out.writeException(reason);
out.writeException(cause);
RestStatus.writeTo(out, status);
}
@ -145,7 +96,7 @@ public class DefaultShardOperationFailedException implements ShardOperationFaile
builder.field("status", status.name());
if (reason != null) {
builder.startObject("reason");
ElasticsearchException.generateThrowableXContent(builder, params, reason);
ElasticsearchException.generateThrowableXContent(builder, params, cause);
builder.endObject();
}
return builder;

48
server/src/main/java/org/elasticsearch/action/support/replication/ReplicationResponse.java
View File

@ -218,13 +218,13 @@ public class ReplicationResponse extends ActionResponse {
'}';
}
public static ShardInfo readShardInfo(StreamInput in) throws IOException {
static ShardInfo readShardInfo(StreamInput in) throws IOException {
ShardInfo shardInfo = new ShardInfo();
shardInfo.readFrom(in);
return shardInfo;
}
public static class Failure implements ShardOperationFailedException, ToXContentObject {
public static class Failure extends ShardOperationFailedException implements ToXContentObject {
private static final String _INDEX = "_index";
private static final String _SHARD = "_shard";
@ -235,37 +235,18 @@ public class ReplicationResponse extends ActionResponse {
private ShardId shardId;
private String nodeId;
private Exception cause;
private RestStatus status;
private boolean primary;
public Failure(ShardId shardId, @Nullable String nodeId, Exception cause, RestStatus status, boolean primary) {
super(shardId.getIndexName(), shardId.getId(), ExceptionsHelper.detailedMessage(cause), status, cause);
this.shardId = shardId;
this.nodeId = nodeId;
this.cause = cause;
this.status = status;
this.primary = primary;
}
Failure() {
}
/**
* @return On what index the failure occurred.
*/
@Override
public String index() {
return shardId.getIndexName();
}
/**
* @return On what shard id the failure occurred.
*/
@Override
public int shardId() {
return shardId.id();
}
public ShardId fullShardId() {
return shardId;
}
@ -278,27 +259,6 @@ public class ReplicationResponse extends ActionResponse {
return nodeId;
}
/**
* @return A text description of the failure
*/
@Override
public String reason() {
return ExceptionsHelper.detailedMessage(cause);
}
/**
* @return The status to report if this failure was a primary failure.
*/
@Override
public RestStatus status() {
return status;
}
@Override
public Throwable getCause() {
return cause;
}
/**
* @return Whether this failure occurred on a primary shard.
* (this only reports true for delete by query)
@ -310,6 +270,8 @@ public class ReplicationResponse extends ActionResponse {
@Override
public void readFrom(StreamInput in) throws IOException {
shardId = ShardId.readShardId(in);
super.shardId = shardId.getId();
super.index = shardId.getIndexName();
nodeId = in.readOptionalString();
cause = in.readException();
status = RestStatus.readFrom(in);

61
server/src/main/java/org/elasticsearch/snapshots/SnapshotShardFailure.java
View File

@ -40,15 +40,11 @@ import java.util.Objects;
/**
* Stores information about failures that occurred during shard snapshotting process
*/
public class SnapshotShardFailure implements ShardOperationFailedException {
private ShardId shardId;
private String reason;
public class SnapshotShardFailure extends ShardOperationFailedException {
@Nullable
private String nodeId;
private RestStatus status;
private ShardId shardId;
private SnapshotShardFailure() {
@ -74,56 +70,9 @@ public class SnapshotShardFailure implements ShardOperationFailedException {
* @param status rest status
*/
private SnapshotShardFailure(@Nullable String nodeId, ShardId shardId, String reason, RestStatus status) {
assert reason != null;
super(shardId.getIndexName(), shardId.id(), reason, status, new IndexShardSnapshotFailedException(shardId, reason));
this.nodeId = nodeId;
this.shardId = shardId;
this.reason = reason;
this.status = status;
}
/**
* Returns index where failure occurred
*
* @return index
*/
@Override
public String index() {
return this.shardId.getIndexName();
}
/**
* Returns shard id where failure occurred
*
* @return shard id
*/
@Override
public int shardId() {
return this.shardId.id();
}
/**
* Returns reason for the failure
*
* @return reason for the failure
*/
@Override
public String reason() {
return this.reason;
}
/**
* Returns {@link RestStatus} corresponding to this failure
*
* @return REST status
*/
@Override
public RestStatus status() {
return status;
}
@Override
public Throwable getCause() {
return new IndexShardSnapshotFailedException(shardId, reason);
}
/**
@ -142,7 +91,7 @@ public class SnapshotShardFailure implements ShardOperationFailedException {
* @param in stream input
* @return shard failure information
*/
public static SnapshotShardFailure readSnapshotShardFailure(StreamInput in) throws IOException {
static SnapshotShardFailure readSnapshotShardFailure(StreamInput in) throws IOException {
SnapshotShardFailure exp = new SnapshotShardFailure();
exp.readFrom(in);
return exp;
@ -152,6 +101,8 @@ public class SnapshotShardFailure implements ShardOperationFailedException {
public void readFrom(StreamInput in) throws IOException {
nodeId = in.readOptionalString();
shardId = ShardId.readShardId(in);
super.shardId = shardId.getId();
super.index = shardId.getIndexName();
reason = in.readString();
status = RestStatus.readFrom(in);
}

25
server/src/test/java/org/elasticsearch/ExceptionsHelperTests.java
View File

@ -174,36 +174,13 @@ public class ExceptionsHelperTests extends ESTestCase {
return new ShardSearchFailure(queryShardException, null);
}
public void testGroupByNullCause() {
ShardOperationFailedException[] failures = new ShardOperationFailedException[] {
new ShardSearchFailure("error", createSearchShardTarget("node0", 0, "index", null)),
new ShardSearchFailure("error", createSearchShardTarget("node1", 1, "index", null)),
new ShardSearchFailure("error", createSearchShardTarget("node1", 1, "index2", null)),
new ShardSearchFailure("error", createSearchShardTarget("node2", 2, "index", "cluster1")),
new ShardSearchFailure("error", createSearchShardTarget("node1", 1, "index", "cluster1")),
new ShardSearchFailure("a different error", createSearchShardTarget("node3", 3, "index", "cluster1"))
};
ShardOperationFailedException[] groupBy = ExceptionsHelper.groupBy(failures);
assertThat(groupBy.length, equalTo(4));
String[] expectedIndices = new String[]{"index", "index2", "cluster1:index", "cluster1:index"};
String[] expectedErrors = new String[]{"error", "error", "error", "a different error"};
int i = 0;
for (ShardOperationFailedException shardOperationFailedException : groupBy) {
assertThat(shardOperationFailedException.reason(), equalTo(expectedErrors[i]));
assertThat(shardOperationFailedException.index(), equalTo(expectedIndices[i++]));
}
}
public void testGroupByNullIndex() {
ShardOperationFailedException[] failures = new ShardOperationFailedException[] {
new ShardSearchFailure("error", null),
new ShardSearchFailure(new IllegalArgumentException("error")),
new ShardSearchFailure(new ParsingException(0, 0, "error", null)),
};
ShardOperationFailedException[] groupBy = ExceptionsHelper.groupBy(failures);
assertThat(groupBy.length, equalTo(3));
assertThat(groupBy.length, equalTo(2));
}
}

76
server/src/test/java/org/elasticsearch/action/ShardOperationFailedExceptionTests.java Normal file
View File

@ -0,0 +1,76 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.elasticsearch.action;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.test.ESTestCase;
import java.io.IOException;
public class ShardOperationFailedExceptionTests extends ESTestCase {
public void testCauseCannotBeNull() {
NullPointerException nullPointerException = expectThrows(NullPointerException.class, () -> new Failure(
randomAlphaOfLengthBetween(3, 10), randomInt(), randomAlphaOfLengthBetween(5, 10), randomFrom(RestStatus.values()), null));
assertEquals("cause cannot be null", nullPointerException.getMessage());
}
public void testStatusCannotBeNull() {
NullPointerException nullPointerException = expectThrows(NullPointerException.class, () -> new Failure(
randomAlphaOfLengthBetween(3, 10), randomInt(), randomAlphaOfLengthBetween(5, 10), null, new IllegalArgumentException()));
assertEquals("status cannot be null", nullPointerException.getMessage());
}
public void testReasonCannotBeNull() {
NullPointerException nullPointerException = expectThrows(NullPointerException.class, () -> new Failure(
randomAlphaOfLengthBetween(3, 10), randomInt(), null, randomFrom(RestStatus.values()), new IllegalArgumentException()));
assertEquals("reason cannot be null", nullPointerException.getMessage());
}
public void testIndexIsNullable() {
new Failure(null, randomInt(), randomAlphaOfLengthBetween(5, 10), randomFrom(RestStatus.values()), new IllegalArgumentException());
}
private static class Failure extends ShardOperationFailedException {
Failure(@Nullable String index, int shardId, String reason, RestStatus status, Throwable cause) {
super(index, shardId, reason, status, cause);
}
@Override
public void readFrom(StreamInput in) throws IOException {
}
@Override
public void writeTo(StreamOutput out) throws IOException {
}
@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
return null;
}
}
}

43
server/src/test/java/org/elasticsearch/rest/action/RestActionsTests.java
View File

@ -202,49 +202,6 @@ public class RestActionsTests extends ESTestCase {
new ShardId(new Index(index, IndexMetaData.INDEX_UUID_NA_VALUE), shardId), clusterAlias, OriginalIndices.NONE);
}
public void testBuildBroadcastShardsHeaderNullCause() throws Exception {
ShardOperationFailedException[] failures = new ShardOperationFailedException[] {
new ShardSearchFailure("error", createSearchShardTarget("node0", 0, "index", null)),
new ShardSearchFailure("error", createSearchShardTarget("node1", 1, "index", null)),
new ShardSearchFailure("error", createSearchShardTarget("node2", 2, "index", "cluster1")),
new ShardSearchFailure("error", createSearchShardTarget("node1", 1, "index", "cluster1")),
new ShardSearchFailure("a different error", createSearchShardTarget("node3", 3, "index", "cluster1"))
};
XContentBuilder builder = JsonXContent.contentBuilder();
builder.prettyPrint();
builder.startObject();
RestActions.buildBroadcastShardsHeader(builder, ToXContent.EMPTY_PARAMS, 12, 3, 0, 9, failures);
builder.endObject();
//TODO the reason is not printed out, as a follow-up we should probably either print it out when the cause is null,
//or even better enforce that the cause can't be null
assertThat(Strings.toString(builder), equalTo("{\n" +
" \"_shards\" : {\n" +
" \"total\" : 12,\n" +
" \"successful\" : 3,\n" +
" \"skipped\" : 0,\n" +
" \"failed\" : 9,\n" +
" \"failures\" : [\n" +
" {\n" +
" \"shard\" : 0,\n" +
" \"index\" : \"index\",\n" +
" \"node\" : \"node0\"\n" +
" },\n" +
" {\n" +
" \"shard\" : 2,\n" +
" \"index\" : \"cluster1:index\",\n" +
" \"node\" : \"node2\"\n" +
" },\n" +
" {\n" +
" \"shard\" : 3,\n" +
" \"index\" : \"cluster1:index\",\n" +
" \"node\" : \"node3\"\n" +
" }\n" +
" ]\n" +
" }\n" +
"}"));
}
@Override
protected NamedXContentRegistry xContentRegistry() {
return xContentRegistry;