From cd56366378c446777d7a05fc7ebbf94df7b1e811 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Wed, 6 Jan 2016 09:49:52 -0500 Subject: [PATCH 1/9] Assert that we fail the correct shard when a replication request fails This commit adds an assertion to TransportReplicationActionTests#runReplicateTest that when a replication request fails, we fail the correct shard. --- .../cluster/action/shard/ShardStateAction.java | 4 ++++ .../replication/TransportReplicationActionTests.java | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/core/src/main/java/org/elasticsearch/cluster/action/shard/ShardStateAction.java b/core/src/main/java/org/elasticsearch/cluster/action/shard/ShardStateAction.java index 58b766e8d84..00a238504f2 100644 --- a/core/src/main/java/org/elasticsearch/cluster/action/shard/ShardStateAction.java +++ b/core/src/main/java/org/elasticsearch/cluster/action/shard/ShardStateAction.java @@ -302,6 +302,10 @@ public class ShardStateAction extends AbstractComponent { this.failure = failure; } + public ShardRouting getShardRouting() { + return shardRouting; + } + @Override public void readFrom(StreamInput in) throws IOException { super.readFrom(in); diff --git a/core/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java b/core/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java index fdcf4b07245..4b5b03857d9 100644 --- a/core/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java +++ b/core/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java @@ -418,6 +418,7 @@ public class TransportReplicationActionTests extends ESTestCase { } } + @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/15790") public void testReplication() throws ExecutionException, InterruptedException { final String index = "test"; final ShardId shardId = new ShardId(index, 0); @@ -441,6 +442,7 @@ public class TransportReplicationActionTests extends ESTestCase { runReplicateTest(shardRoutingTable, assignedReplicas, totalShards); } + @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/15790") public void testReplicationWithShadowIndex() throws ExecutionException, InterruptedException { final String index = "test"; final ShardId shardId = new ShardId(index, 0); @@ -511,6 +513,12 @@ public class TransportReplicationActionTests extends ESTestCase { transport.clear(); assertEquals(1, shardFailedRequests.length); CapturingTransport.CapturedRequest shardFailedRequest = shardFailedRequests[0]; + // get the shard the request was sent to + ShardRouting routing = clusterService.state().getRoutingNodes().node(capturedRequest.node.id()).get(request.shardId.id()); + // and the shard that was requested to be failed + ShardStateAction.ShardRoutingEntry shardRoutingEntry = (ShardStateAction.ShardRoutingEntry)shardFailedRequest.request; + // the shard the request was sent to and the shard to be failed should be the same + assertTrue(shardRoutingEntry.getShardRouting().isSameAllocation(routing)); failures.add(shardFailedRequest); transport.handleResponse(shardFailedRequest.requestId, TransportResponse.Empty.INSTANCE); } From 247ce06fc3bd891b3b15a82ff1482ad6cd573bfb Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Wed, 6 Jan 2016 18:00:00 +0100 Subject: [PATCH 2/9] percolator: if size is 0 then use TotalHitCountCollector Fixes PercolateIT#testPercolateSizingWithQueryAndFilter test --- .../java/org/elasticsearch/percolator/PercolatorService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/org/elasticsearch/percolator/PercolatorService.java b/core/src/main/java/org/elasticsearch/percolator/PercolatorService.java index fd07d54a7d1..e6ffa313e83 100644 --- a/core/src/main/java/org/elasticsearch/percolator/PercolatorService.java +++ b/core/src/main/java/org/elasticsearch/percolator/PercolatorService.java @@ -254,7 +254,7 @@ public class PercolatorService extends AbstractComponent { } PercolatorQuery percolatorQuery = builder.build(); - if (context.isOnlyCount()) { + if (context.isOnlyCount() || context.size() == 0) { TotalHitCountCollector collector = new TotalHitCountCollector(); context.searcher().search(percolatorQuery, MultiCollector.wrap(collector, aggregatorCollector)); if (aggregatorCollector != null) { From 23a64f8214a990907ece74d00294f7dc3b25ed92 Mon Sep 17 00:00:00 2001 From: Damien Alexandre Date: Wed, 6 Jan 2016 18:26:03 +0100 Subject: [PATCH 3/9] Fix french analyzer elision token filter doc Fix #15774 --- .../analysis/analyzers/lang-analyzer.asciidoc | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/reference/analysis/analyzers/lang-analyzer.asciidoc b/docs/reference/analysis/analyzers/lang-analyzer.asciidoc index 69388ffa7a1..d1eb1acff9e 100644 --- a/docs/reference/analysis/analyzers/lang-analyzer.asciidoc +++ b/docs/reference/analysis/analyzers/lang-analyzer.asciidoc @@ -619,11 +619,13 @@ The `french` analyzer could be reimplemented as a `custom` analyzer as follows: "analysis": { "filter": { "french_elision": { - "type": "elision", - "articles": [ "l", "m", "t", "qu", "n", "s", - "j", "d", "c", "jusqu", "quoiqu", - "lorsqu", "puisqu" - ] + "type": "elision", + "articles_case": true, + "articles": [ + "l", "m", "t", "qu", "n", "s", + "j", "d", "c", "jusqu", "quoiqu", + "lorsqu", "puisqu" + ] }, "french_stop": { "type": "stop", From 81cffd1be367e0ada344bc614bb5f3bbf559257e Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Wed, 6 Jan 2016 18:30:04 +0100 Subject: [PATCH 4/9] test: mute test --- .../org/elasticsearch/percolator/ConcurrentPercolatorIT.java | 1 + 1 file changed, 1 insertion(+) diff --git a/core/src/test/java/org/elasticsearch/percolator/ConcurrentPercolatorIT.java b/core/src/test/java/org/elasticsearch/percolator/ConcurrentPercolatorIT.java index a7f50d33608..031b305864c 100644 --- a/core/src/test/java/org/elasticsearch/percolator/ConcurrentPercolatorIT.java +++ b/core/src/test/java/org/elasticsearch/percolator/ConcurrentPercolatorIT.java @@ -148,6 +148,7 @@ public class ConcurrentPercolatorIT extends ESIntegTestCase { assertThat(assertionError + " should be null", assertionError, nullValue()); } + @AwaitsFix(bugUrl = "reproduces= -Dtests.seed=DA9C1BDEB045305C") public void testConcurrentAddingAndPercolating() throws Exception { assertAcked(prepareCreate("index").addMapping("type", "field1", "type=string", "field2", "type=string")); ensureGreen(); From 75106daf9c321b9282a283bd0511f30728213f01 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Wed, 6 Jan 2016 10:07:08 -0500 Subject: [PATCH 5/9] Only fail the relocation target when a replication request on it fails This commit addresses an issue when handling a failed replication request against a relocating target shard. Namely, if a replication request fails against the target of a relocation we currently fail both the source and the target. This leads to an unnecessary recovery. Instead, only the target of the relocation should be failed. --- .../support/replication/TransportReplicationAction.java | 7 ++++--- .../replication/TransportReplicationActionTests.java | 2 -- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/action/support/replication/TransportReplicationAction.java b/core/src/main/java/org/elasticsearch/action/support/replication/TransportReplicationAction.java index 6fd7da91645..0014404057f 100644 --- a/core/src/main/java/org/elasticsearch/action/support/replication/TransportReplicationAction.java +++ b/core/src/main/java/org/elasticsearch/action/support/replication/TransportReplicationAction.java @@ -844,11 +844,11 @@ public abstract class TransportReplicationAction Date: Wed, 6 Jan 2016 10:55:38 -0500 Subject: [PATCH 6/9] Assert that replication requests are sent to the correct shard copies This commit adds tighter assertions in TransportReplicationActionTests#runReplicateTest that replication requests are sent to the correct shard copies. --- .../TransportReplicationActionTests.java | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/core/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java b/core/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java index 43623437b27..e586420a6ce 100644 --- a/core/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java +++ b/core/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java @@ -67,6 +67,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.HashSet; import java.util.List; +import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; @@ -77,6 +78,7 @@ import static org.elasticsearch.action.support.replication.ClusterStateCreationU import static org.elasticsearch.action.support.replication.ClusterStateCreationUtils.stateWithStartedPrimary; import static org.hamcrest.Matchers.arrayWithSize; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; @@ -486,7 +488,37 @@ public class TransportReplicationActionTests extends ESTestCase { replicationPhase.run(); final CapturingTransport.CapturedRequest[] capturedRequests = transport.capturedRequests(); transport.clear(); + assertThat(capturedRequests.length, equalTo(assignedReplicas)); + Set nodesSentTo = new HashSet<>(); + boolean executeOnReplica = + action.shouldExecuteReplication(clusterService.state().getMetaData().index(shardId.getIndex()).getSettings()); + for (CapturingTransport.CapturedRequest capturedRequest : capturedRequests) { + // no duplicate requests + assertTrue(nodesSentTo.add(capturedRequest.node.getId())); + // the request is hitting the correct shard + Request replicationRequest = (Request)capturedRequest.request; + assertEquals(request.shardId, replicationRequest.shardId); + } + + // requests were sent to the correct shard copies + List shards = + clusterService.state().getRoutingTable().index(shardId.getIndex()).shard(shardId.id()).shards(); + for (ShardRouting shard : shards) { + if (!shard.primary() && !executeOnReplica) { + continue; + } + if (shard.unassigned()) { + continue; + } + if (!clusterService.state().getNodes().localNodeId().equals(shard.currentNodeId())) { + assertThat(nodesSentTo, hasItem(shard.currentNodeId())); + } + if (shard.relocating()) { + assertThat(nodesSentTo, hasItem(shard.relocatingNodeId())); + } + } + if (assignedReplicas > 0) { assertThat("listener is done, but there are outstanding replicas", listener.isDone(), equalTo(false)); } From c291c1714295dd0ab7024f50518fc39fa21d742d Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Wed, 6 Jan 2016 11:40:07 -0500 Subject: [PATCH 7/9] Cleanup TransportReplicationActionTests#runReplicateTest This commit cleans up some of the assertions in TransportReplicationActionTests#runReplicateTest: - use a Map to track actual vs. expected requests - assert that no request was sent to the local node - use RoutingTable#shardRoutingTable convenience method - explicitly use false in boolean conditions - clarify requests are expected on replica shards when assigned and execution on replicas is true - test ShardRouting equality when checking the failed shard request --- .../TransportReplicationActionTests.java | 30 +++++++++++-------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/core/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java b/core/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java index e586420a6ce..4f052e9fbd7 100644 --- a/core/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java +++ b/core/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java @@ -65,9 +65,9 @@ import org.junit.BeforeClass; import java.io.IOException; import java.util.ArrayList; +import java.util.HashMap; import java.util.HashSet; import java.util.List; -import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; @@ -76,10 +76,13 @@ import java.util.concurrent.atomic.AtomicInteger; import static org.elasticsearch.action.support.replication.ClusterStateCreationUtils.state; import static org.elasticsearch.action.support.replication.ClusterStateCreationUtils.stateWithStartedPrimary; +import static org.hamcrest.CoreMatchers.not; import static org.hamcrest.Matchers.arrayWithSize; +import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; @@ -490,35 +493,38 @@ public class TransportReplicationActionTests extends ESTestCase { transport.clear(); assertThat(capturedRequests.length, equalTo(assignedReplicas)); - Set nodesSentTo = new HashSet<>(); + HashMap nodesSentTo = new HashMap<>(); boolean executeOnReplica = action.shouldExecuteReplication(clusterService.state().getMetaData().index(shardId.getIndex()).getSettings()); for (CapturingTransport.CapturedRequest capturedRequest : capturedRequests) { // no duplicate requests - assertTrue(nodesSentTo.add(capturedRequest.node.getId())); + Request replicationRequest = (Request) capturedRequest.request; + assertNull(nodesSentTo.put(capturedRequest.node.getId(), replicationRequest)); // the request is hitting the correct shard - Request replicationRequest = (Request)capturedRequest.request; assertEquals(request.shardId, replicationRequest.shardId); } + // no request was sent to the local node + assertThat(nodesSentTo.keySet(), not(hasItem(clusterService.state().getNodes().localNodeId()))); + // requests were sent to the correct shard copies - List shards = - clusterService.state().getRoutingTable().index(shardId.getIndex()).shard(shardId.id()).shards(); - for (ShardRouting shard : shards) { - if (!shard.primary() && !executeOnReplica) { + for (ShardRouting shard : clusterService.state().getRoutingTable().shardRoutingTable(shardId.getIndex(), shardId.id())) { + if (shard.primary() == false && executeOnReplica == false) { continue; } if (shard.unassigned()) { continue; } - if (!clusterService.state().getNodes().localNodeId().equals(shard.currentNodeId())) { - assertThat(nodesSentTo, hasItem(shard.currentNodeId())); + if (shard.primary() == false) { + nodesSentTo.remove(shard.currentNodeId()); } if (shard.relocating()) { - assertThat(nodesSentTo, hasItem(shard.relocatingNodeId())); + nodesSentTo.remove(shard.relocatingNodeId()); } } + assertThat(nodesSentTo.entrySet(), is(empty())); + if (assignedReplicas > 0) { assertThat("listener is done, but there are outstanding replicas", listener.isDone(), equalTo(false)); } @@ -548,7 +554,7 @@ public class TransportReplicationActionTests extends ESTestCase { // and the shard that was requested to be failed ShardStateAction.ShardRoutingEntry shardRoutingEntry = (ShardStateAction.ShardRoutingEntry)shardFailedRequest.request; // the shard the request was sent to and the shard to be failed should be the same - assertTrue(shardRoutingEntry.getShardRouting().isSameAllocation(routing)); + assertEquals(shardRoutingEntry.getShardRouting(), routing); failures.add(shardFailedRequest); transport.handleResponse(shardFailedRequest.requestId, TransportResponse.Empty.INSTANCE); } From bb4d857e447b64c31812fbbde0af7619b4518aed Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Wed, 6 Jan 2016 12:51:01 -0500 Subject: [PATCH 8/9] Redundant assertion in TransportReplicationActionTests#runReplicateTest --- .../support/replication/TransportReplicationActionTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/core/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java b/core/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java index 4f052e9fbd7..6ccf48c7930 100644 --- a/core/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java +++ b/core/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java @@ -492,7 +492,6 @@ public class TransportReplicationActionTests extends ESTestCase { final CapturingTransport.CapturedRequest[] capturedRequests = transport.capturedRequests(); transport.clear(); - assertThat(capturedRequests.length, equalTo(assignedReplicas)); HashMap nodesSentTo = new HashMap<>(); boolean executeOnReplica = action.shouldExecuteReplication(clusterService.state().getMetaData().index(shardId.getIndex()).getSettings()); From 04b79c112fc1c05bc7550032742cdb25edef10d2 Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Wed, 6 Jan 2016 19:09:12 +0100 Subject: [PATCH 9/9] test: unmuted test test failed, because now the percolator returns upto 10 matches whereas before this was unbounded. The test has been updated to take this in account by checking the total count instead of the number of matches --- .../elasticsearch/percolator/ConcurrentPercolatorIT.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/core/src/test/java/org/elasticsearch/percolator/ConcurrentPercolatorIT.java b/core/src/test/java/org/elasticsearch/percolator/ConcurrentPercolatorIT.java index 031b305864c..178f070927c 100644 --- a/core/src/test/java/org/elasticsearch/percolator/ConcurrentPercolatorIT.java +++ b/core/src/test/java/org/elasticsearch/percolator/ConcurrentPercolatorIT.java @@ -148,7 +148,6 @@ public class ConcurrentPercolatorIT extends ESIntegTestCase { assertThat(assertionError + " should be null", assertionError, nullValue()); } - @AwaitsFix(bugUrl = "reproduces= -Dtests.seed=DA9C1BDEB045305C") public void testConcurrentAddingAndPercolating() throws Exception { assertAcked(prepareCreate("index").addMapping("type", "field1", "type=string", "field2", "type=string")); ensureGreen(); @@ -252,7 +251,7 @@ public class ConcurrentPercolatorIT extends ESIntegTestCase { .setSource(onlyField1Doc).execute().actionGet(); assertNoFailures(response); assertThat(response.getSuccessfulShards(), equalTo(response.getTotalShards())); - assertThat(response.getMatches().length, greaterThanOrEqualTo(atLeastExpected)); + assertThat(response.getCount(), greaterThanOrEqualTo((long) atLeastExpected)); break; case 1: atLeastExpected = type2.get(); @@ -260,7 +259,7 @@ public class ConcurrentPercolatorIT extends ESIntegTestCase { .setSource(onlyField2Doc).execute().actionGet(); assertNoFailures(response); assertThat(response.getSuccessfulShards(), equalTo(response.getTotalShards())); - assertThat(response.getMatches().length, greaterThanOrEqualTo(atLeastExpected)); + assertThat(response.getCount(), greaterThanOrEqualTo((long) atLeastExpected)); break; case 2: atLeastExpected = type3.get(); @@ -268,7 +267,7 @@ public class ConcurrentPercolatorIT extends ESIntegTestCase { .setSource(field1AndField2Doc).execute().actionGet(); assertNoFailures(response); assertThat(response.getSuccessfulShards(), equalTo(response.getTotalShards())); - assertThat(response.getMatches().length, greaterThanOrEqualTo(atLeastExpected)); + assertThat(response.getCount(), greaterThanOrEqualTo((long) atLeastExpected)); break; } }