Applied minor feedback.

Wrapped the original primary shard failure reason on replica failures when an entire shard group failed.
elastic · Dec 9, 2014 · c99bb0f · c99bb0f
1 parent d88eec5
commit c99bb0f
Show file tree

Hide file tree

Showing 3 changed files with 13 additions and 20 deletions.
diff --git a/...rg/elasticsearch/action/support/replication/TransportIndexReplicationOperationAction.java b/...rg/elasticsearch/action/support/replication/TransportIndexReplicationOperationAction.java
@@ -36,7 +36,6 @@
 import org.elasticsearch.cluster.routing.ShardIterator;
 import org.elasticsearch.cluster.routing.ShardRouting;
 import org.elasticsearch.common.settings.Settings;
-import org.elasticsearch.index.shard.ShardId;
 import org.elasticsearch.threadpool.ThreadPool;
 
 import java.util.ArrayList;
@@ -110,7 +109,7 @@ public void onFailure(Throwable e) {
                     int index = indexCounter.getAndIncrement();
                     if (accumulateExceptions()) {
                         shardsResponses.set(index, new ShardActionResult(
-                                new DefaultShardOperationFailedException(request.index(), shardIt.shardId().id(), e)));
+                                new DefaultShardOperationFailedException(request.index(), shardIt.shardId().id(), e), shardIt));
                     }
                     returnIfNeeded();
                 }
@@ -135,21 +134,12 @@ private void returnIfNeeded() {
                                 // The failure doesn't include the node id, maybe add it to ShardOperationFailedException...
                                 ShardOperationFailedException sf = shardActionResult.shardFailure;
 
-                                ShardIterator thisShardIterator = null;
-                                ShardId shardId = new ShardId(sf.index(), sf.shardId());
-                                for (ShardIterator shardIterator : groups) {
-                                    if (shardIterator.shardId().equals(shardId)) {
-                                        thisShardIterator = shardIterator;
-                                        break;
-                                    }
-                                }
-
-                                assert thisShardIterator != null;
-                                for (ShardRouting shardRouting = thisShardIterator.nextOrNull(); shardRouting != null; shardRouting = thisShardIterator.nextOrNull()) {
+                                ShardIterator shardIterator = shardActionResult.shardIterator;
+                                for (ShardRouting shardRouting = shardIterator.nextOrNull(); shardRouting != null; shardRouting = shardIterator.nextOrNull()) {
                                     if (shardRouting.primary()) {
                                         failureList.add(new Failure(sf.index(), sf.shardId(), shardRouting.currentNodeId(), sf.reason(), sf.status(), true));
                                     } else {
-                                        failureList.add(new Failure(sf.index(), sf.shardId(), shardRouting.currentNodeId(), "Not executed because operation failed on primary shard", sf.status(), false));
+                                        failureList.add(new Failure(sf.index(), sf.shardId(), shardRouting.currentNodeId(), "Failed to execute on replica shard: " + sf.reason(), sf.status(), false));
                                     }
                                 }
                             } else {
@@ -205,16 +195,19 @@ private class ShardActionResult {
 
         private final ShardResponse shardResponse;
         private final ShardOperationFailedException shardFailure;
+        private final ShardIterator shardIterator;
 
         private ShardActionResult(ShardResponse shardResponse) {
             assert shardResponse != null;
             this.shardResponse = shardResponse;
             this.shardFailure = null;
+            this.shardIterator = null;
         }
 
-        private ShardActionResult(ShardOperationFailedException shardOperationFailedException) {
+        private ShardActionResult(ShardOperationFailedException shardOperationFailedException, ShardIterator shardIterator) {
             assert shardOperationFailedException != null;
             this.shardFailure = shardOperationFailedException;
+            this.shardIterator = shardIterator;
             this.shardResponse = null;
         }
 

diff --git a/src/main/java/org/elasticsearch/rest/action/index/RestIndexAction.java b/src/main/java/org/elasticsearch/rest/action/index/RestIndexAction.java
@@ -115,9 +115,9 @@ public RestResponse buildResponse(IndexResponse response, XContentBuilder builde
                 builder.field(Fields._INDEX, response.getIndex())
                         .field(Fields._TYPE, response.getType())
                         .field(Fields._ID, response.getId())
-                        .field(Fields._VERSION, response.getVersion())
-                        .value(shardInfo)
-                        .field(Fields.CREATED, response.isCreated());
+                        .field(Fields._VERSION, response.getVersion());
+                shardInfo.toXContent(builder, request);
+                builder.field(Fields.CREATED, response.isCreated());
                 builder.endObject();
                 RestStatus status = shardInfo.status();
                 if (response.isCreated()) {

diff --git a/src/main/java/org/elasticsearch/rest/action/update/RestUpdateAction.java b/src/main/java/org/elasticsearch/rest/action/update/RestUpdateAction.java
@@ -132,9 +132,9 @@ public RestResponse buildResponse(UpdateResponse response, XContentBuilder build
                 builder.field(Fields._INDEX, response.getIndex())
                         .field(Fields._TYPE, response.getType())
                         .field(Fields._ID, response.getId())
-                        .field(Fields._VERSION, response.getVersion())
-                        .value(shardInfo);
+                        .field(Fields._VERSION, response.getVersion());
 
+                shardInfo.toXContent(builder, request);
                 if (response.getGetResult() != null) {
                     builder.startObject(Fields.GET);
                     response.getGetResult().toXContentEmbedded(builder, request);