From 2beda3953d75b387d30cf0fba1095f4b766b0b2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Thu, 2 Apr 2015 11:10:38 +0200 Subject: [PATCH] Remove expansion of empty index arguments in RoutingTable RoutingTables activePrimaryShardsGrouped(), allActiveShardsGrouped() and allAssignedShardsGrouped() methods treated empty index array input parameters as meaning "all" indices and expanded to the routing maps keyset. However, the expansion of index names is now already done in MetaData#concreteIndices(). Returning an empty index name list here when a wildcard pattern didn't match any index name could lead to problems like #9081 because the RoutingTable still expanded this list of names to "_all". In case of e.g. the recovery endpoint this could lead to problems. Closes #9081 Closes #10148 --- .../test/indices.recovery/10_basic.yaml | 41 +++ .../test/indices.refresh/10_basic.yaml | 58 ++++ .../test/indices.stats/10_index.yaml | 10 + .../cluster/routing/RoutingTable.java | 73 ++---- .../action/IndicesRequestTests.java | 7 +- .../cluster/routing/RoutingTableTest.java | 248 ++++++++++++++++++ 6 files changed, 381 insertions(+), 56 deletions(-) create mode 100644 rest-api-spec/test/indices.refresh/10_basic.yaml create mode 100644 src/test/java/org/elasticsearch/cluster/routing/RoutingTableTest.java diff --git a/rest-api-spec/test/indices.recovery/10_basic.yaml b/rest-api-spec/test/indices.recovery/10_basic.yaml index 4db9f80813c..8204ed59a25 100644 --- a/rest-api-spec/test/indices.recovery/10_basic.yaml +++ b/rest-api-spec/test/indices.recovery/10_basic.yaml @@ -37,3 +37,44 @@ - gte: { test_1.shards.0.translog.total_time_in_millis: 0 } - gte: { test_1.shards.0.start.check_index_time_in_millis: 0 } - gte: { test_1.shards.0.start.total_time_in_millis: 0 } +--- +"Indices recovery test index name not matching": + + - do: + indices.create: + index: test_1 + body: + settings: + index: + number_of_replicas: 0 + + - do: + cluster.health: + wait_for_status: green + + - do: + + catch: missing + indices.recovery: + index: [foobar] + +--- +"Indices recovery test, wildcard not matching any index": + + - do: + indices.create: + index: test_1 + body: + settings: + index: + number_of_replicas: 0 + + - do: + cluster.health: + wait_for_status: green + + - do: + indices.recovery: + index: [v*] + + - match: { $body: {} } diff --git a/rest-api-spec/test/indices.refresh/10_basic.yaml b/rest-api-spec/test/indices.refresh/10_basic.yaml new file mode 100644 index 00000000000..6e493a0cce9 --- /dev/null +++ b/rest-api-spec/test/indices.refresh/10_basic.yaml @@ -0,0 +1,58 @@ +--- +setup: + - do: + indices.create: + index: test_1 + body: + settings: + index: + number_of_replicas: 0 + number_of_shards: 5 + + - do: + indices.create: + index: test_2 + body: + settings: + index: + number_of_replicas: 0 + number_of_shards: 5 + + - do: + cluster.health: + wait_for_status: green + +--- +"Indices refresh test _all": + + - do: + indices.refresh: + index: [_all] + + - match: { _shards.total: 10 } + - match: { _shards.successful: 10 } + - match: { _shards.failed: 0 } + +--- +"Indices refresh test empty array": + + + - do: + indices.refresh: + index: [] + + - match: { _shards.total: 10 } + - match: { _shards.successful: 10 } + - match: { _shards.failed: 0 } + +--- +"Indices refresh test no-match wildcard": + + - do: + indices.refresh: + index: [bla*] + + - match: { _shards.total: 0 } + - match: { _shards.successful: 0 } + - match: { _shards.failed: 0 } + diff --git a/rest-api-spec/test/indices.stats/10_index.yaml b/rest-api-spec/test/indices.stats/10_index.yaml index 7bb596b59e6..0710b63e5bb 100644 --- a/rest-api-spec/test/indices.stats/10_index.yaml +++ b/rest-api-spec/test/indices.stats/10_index.yaml @@ -60,6 +60,16 @@ setup: - is_true: indices.test1 - is_true: indices.test2 +--- +"Index - star, no match": + - do: + indices.stats: { index: 'bla*' } + + - match: { _shards.total: 0 } + - is_true: _all + - is_false: indices.test1 + - is_false: indices.test2 + --- "Index - one index": - do: diff --git a/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java b/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java index 6f44a1d11fc..9f1b5db6c6b 100644 --- a/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java +++ b/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java @@ -117,61 +117,39 @@ public class RoutingTable implements Iterable { } /** - * All the shards (replicas) for the provided indices. + * All the shards (replicas) for all indices in this routing table. * - * @param indices The indices to return all the shards (replicas), can be null or empty array to indicate all indices - * @return All the shards matching the specific index - * @throws IndexMissingException If an index passed does not exists + * @return All the shards */ - public List allShards(String... indices) throws IndexMissingException { + public List allShards() throws IndexMissingException { List shards = Lists.newArrayList(); - if (indices == null || indices.length == 0) { - indices = indicesRouting.keySet().toArray(new String[indicesRouting.keySet().size()]); - } + String[] indices = indicesRouting.keySet().toArray(new String[indicesRouting.keySet().size()]); for (String index : indices) { - IndexRoutingTable indexRoutingTable = index(index); - if (indexRoutingTable == null) { - throw new IndexMissingException(new Index(index)); - } - for (IndexShardRoutingTable indexShardRoutingTable : indexRoutingTable) { - for (ShardRouting shardRouting : indexShardRoutingTable) { - shards.add(shardRouting); - } - } + List allShardsIndex = allShards(index); + shards.addAll(allShardsIndex); } return shards; } /** - * All the shards (primary + replicas) for the provided indices grouped (each group is a single element, consisting - * of the shard). This is handy for components that expect to get group iterators, but still want in some - * cases to iterate over all the shards (and not just one shard in replication group). + * All the shards (replicas) for the provided index. * - * @param indices The indices to return all the shards (replicas), can be null or empty array to indicate all indices - * @return All the shards grouped into a single shard element group each - * @throws IndexMissingException If an index passed does not exists - * @see IndexRoutingTable#groupByAllIt() + * @param index The index to return all the shards (replicas). + * @return All the shards matching the specific index + * @throws IndexMissingException If the index passed does not exists */ - public GroupShardsIterator allShardsGrouped(String... indices) throws IndexMissingException { - // use list here since we need to maintain identity across shards - ArrayList set = new ArrayList<>(); - if (indices == null || indices.length == 0) { - indices = indicesRouting.keySet().toArray(new String[indicesRouting.keySet().size()]); + public List allShards(String index) throws IndexMissingException { + List shards = Lists.newArrayList(); + IndexRoutingTable indexRoutingTable = index(index); + if (indexRoutingTable == null) { + throw new IndexMissingException(new Index(index)); } - for (String index : indices) { - IndexRoutingTable indexRoutingTable = index(index); - if (indexRoutingTable == null) { - continue; - // we simply ignore indices that don't exists (make sense for operations that use it currently) -// throw new IndexMissingException(new Index(index)); - } - for (IndexShardRoutingTable indexShardRoutingTable : indexRoutingTable) { - for (ShardRouting shardRouting : indexShardRoutingTable) { - set.add(shardRouting.shardsIt()); - } + for (IndexShardRoutingTable indexShardRoutingTable : indexRoutingTable) { + for (ShardRouting shardRouting : indexShardRoutingTable) { + shards.add(shardRouting); } } - return new GroupShardsIterator(set); + return shards; } public GroupShardsIterator allActiveShardsGrouped(String[] indices, boolean includeEmpty) throws IndexMissingException { @@ -188,15 +166,11 @@ public class RoutingTable implements Iterable { public GroupShardsIterator allActiveShardsGrouped(String[] indices, boolean includeEmpty, boolean includeRelocationTargets) throws IndexMissingException { // use list here since we need to maintain identity across shards ArrayList set = new ArrayList<>(); - if (indices == null || indices.length == 0) { - indices = indicesRouting.keySet().toArray(new String[indicesRouting.keySet().size()]); - } for (String index : indices) { IndexRoutingTable indexRoutingTable = index(index); if (indexRoutingTable == null) { continue; // we simply ignore indices that don't exists (make sense for operations that use it currently) -// throw new IndexMissingException(new Index(index)); } for (IndexShardRoutingTable indexShardRoutingTable : indexRoutingTable) { for (ShardRouting shardRouting : indexShardRoutingTable) { @@ -228,15 +202,11 @@ public class RoutingTable implements Iterable { public GroupShardsIterator allAssignedShardsGrouped(String[] indices, boolean includeEmpty, boolean includeRelocationTargets) throws IndexMissingException { // use list here since we need to maintain identity across shards ArrayList set = new ArrayList<>(); - if (indices == null || indices.length == 0) { - indices = indicesRouting.keySet().toArray(new String[indicesRouting.keySet().size()]); - } for (String index : indices) { IndexRoutingTable indexRoutingTable = index(index); if (indexRoutingTable == null) { continue; // we simply ignore indices that don't exists (make sense for operations that use it currently) -// throw new IndexMissingException(new Index(index)); } for (IndexShardRoutingTable indexShardRoutingTable : indexRoutingTable) { for (ShardRouting shardRouting : indexShardRoutingTable) { @@ -259,7 +229,7 @@ public class RoutingTable implements Iterable { * of the primary shard). This is handy for components that expect to get group iterators, but still want in some * cases to iterate over all primary shards (and not just one shard in replication group). * - * @param indices The indices to return all the shards (replicas), can be null or empty array to indicate all indices + * @param indices The indices to return all the shards (replicas) * @return All the primary shards grouped into a single shard element group each * @throws IndexMissingException If an index passed does not exists * @see IndexRoutingTable#groupByAllIt() @@ -267,9 +237,6 @@ public class RoutingTable implements Iterable { public GroupShardsIterator activePrimaryShardsGrouped(String[] indices, boolean includeEmpty) throws IndexMissingException { // use list here since we need to maintain identity across shards ArrayList set = new ArrayList<>(); - if (indices == null || indices.length == 0) { - indices = indicesRouting.keySet().toArray(new String[indicesRouting.keySet().size()]); - } for (String index : indices) { IndexRoutingTable indexRoutingTable = index(index); if (indexRoutingTable == null) { diff --git a/src/test/java/org/elasticsearch/action/IndicesRequestTests.java b/src/test/java/org/elasticsearch/action/IndicesRequestTests.java index 84323360b60..e7a0f9d2e62 100644 --- a/src/test/java/org/elasticsearch/action/IndicesRequestTests.java +++ b/src/test/java/org/elasticsearch/action/IndicesRequestTests.java @@ -52,6 +52,7 @@ import org.elasticsearch.action.admin.indices.settings.put.UpdateSettingsAction; import org.elasticsearch.action.admin.indices.settings.put.UpdateSettingsRequest; import org.elasticsearch.action.admin.indices.stats.IndicesStatsAction; import org.elasticsearch.action.admin.indices.stats.IndicesStatsRequest; +import org.elasticsearch.action.admin.indices.stats.IndicesStatsResponse; import org.elasticsearch.action.admin.indices.validate.query.ValidateQueryAction; import org.elasticsearch.action.admin.indices.validate.query.ValidateQueryRequest; import org.elasticsearch.action.bulk.BulkAction; @@ -92,6 +93,7 @@ import org.elasticsearch.action.update.UpdateRequest; import org.elasticsearch.action.update.UpdateResponse; import org.elasticsearch.cluster.settings.ClusterDynamicSettings; import org.elasticsearch.cluster.settings.DynamicSettings; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.ImmutableSettings; import org.elasticsearch.common.settings.Settings; @@ -865,7 +867,7 @@ public class IndicesRequestTests extends ElasticsearchIntegrationTest { ((InterceptingTransportService) transportService).clearInterceptedActions(); } } - + private static void interceptTransportActions(String... actions) { Iterable transportServices = internalCluster().getInstances(TransportService.class); for (TransportService transportService : transportServices) { @@ -892,8 +894,7 @@ public class IndicesRequestTests extends ElasticsearchIntegrationTest { private final Map> requests = new HashMap<>(); @Inject - public InterceptingTransportService(Settings settings, Transport transport, ThreadPool threadPool, - NodeSettingsService nodeSettingsService, @ClusterDynamicSettings DynamicSettings dynamicSettings) { + public InterceptingTransportService(Settings settings, Transport transport, ThreadPool threadPool) { super(settings, transport, threadPool); } diff --git a/src/test/java/org/elasticsearch/cluster/routing/RoutingTableTest.java b/src/test/java/org/elasticsearch/cluster/routing/RoutingTableTest.java new file mode 100644 index 00000000000..4a945f7e92f --- /dev/null +++ b/src/test/java/org/elasticsearch/cluster/routing/RoutingTableTest.java @@ -0,0 +1,248 @@ +/* + * 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.cluster.routing; + +import org.elasticsearch.Version; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.metadata.IndexMetaData; +import org.elasticsearch.cluster.metadata.MetaData; +import org.elasticsearch.cluster.node.DiscoveryNodes; +import org.elasticsearch.cluster.node.DiscoveryNodes.Builder; +import org.elasticsearch.cluster.routing.allocation.AllocationService; +import org.elasticsearch.cluster.routing.allocation.RoutingAllocation; +import org.elasticsearch.common.settings.ImmutableSettings; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.indices.IndexMissingException; +import org.elasticsearch.test.ElasticsearchAllocationTestCase; +import org.junit.Before; +import org.junit.Test; + +import static org.hamcrest.Matchers.nullValue; + +import static org.elasticsearch.cluster.routing.ShardRoutingState.INITIALIZING; +import static org.elasticsearch.common.settings.ImmutableSettings.settingsBuilder; +import static org.hamcrest.Matchers.is; + +public class RoutingTableTest extends ElasticsearchAllocationTestCase { + + private static final String TEST_INDEX_1 = "test1"; + private static final String TEST_INDEX_2 = "test2"; + private RoutingTable emptyRoutingTable; + private RoutingTable testRoutingTable; + private int numberOfShards; + private int numberOfReplicas; + private int shardsPerIndex; + private int totalNumberOfShards; + private final static Settings DEFAULT_SETTINGS = ImmutableSettings.builder().put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT).build(); + private final AllocationService ALLOCATION_SERVICE = createAllocationService(settingsBuilder() + .put("cluster.routing.allocation.concurrent_recoveries", 10) + .put("cluster.routing.allocation.node_initial_primaries_recoveries", 10) + .build()); + private ClusterState clusterState; + + @Before + public void setUp() throws Exception { + super.setUp(); + this.numberOfShards = randomIntBetween(1, 5); + this.numberOfReplicas = randomIntBetween(1, 5); + this.shardsPerIndex = this.numberOfShards * (this.numberOfReplicas + 1); + this.totalNumberOfShards = this.shardsPerIndex * 2; + logger.info("Setup test with " + this.numberOfShards + " shards and " + this.numberOfReplicas + " replicas."); + this.emptyRoutingTable = new RoutingTable.Builder().build(); + MetaData metaData = MetaData.builder() + .put(createIndexMetaData(TEST_INDEX_1)) + .put(createIndexMetaData(TEST_INDEX_2)) + .build(); + + this.testRoutingTable = new RoutingTable.Builder() + .add(new IndexRoutingTable.Builder(TEST_INDEX_1).initializeAsNew(metaData.index(TEST_INDEX_1)).build()) + .add(new IndexRoutingTable.Builder(TEST_INDEX_2).initializeAsNew(metaData.index(TEST_INDEX_2)).build()) + .build(); + this.clusterState = ClusterState.builder(org.elasticsearch.cluster.ClusterName.DEFAULT).metaData(metaData).routingTable(testRoutingTable).build(); + } + + /** + * puts primary shard routings into initializing state + */ + private void initPrimaries() { + logger.info("adding " + (this.numberOfReplicas + 1) + " nodes and performing rerouting"); + Builder discoBuilder = DiscoveryNodes.builder(); + for (int i=0; i