From 93500ef4ad755010a37469a9ffcd2cb4106ff23e Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 15 Oct 2018 14:39:57 -0400 Subject: [PATCH 1/2] Do not update number of replicas on no indices Today when submitting an update settings request to update the number of replicas with a wildcard that does not match any indices and allow no indices is set to true, the request ends up being interpreted as updating the number of replicas for all indices. That is, consider the following sequence: PUT /test-index { "settings": { "index.number_of_replicas": 0 } } PUT /non-existent-*/_settings?expand_wildcards=open&allow_no_indices=true { "settings": { "index.number_of_replicas": 1 } } GET /test-index/_settings The latter will show that the number of replicas on test-index is now one. This is surprising, and should be considered a bug. The underlying problem here is treating no indices in the underlying methods used to update the routing table and the metadata as meaning all indices. This commit takes away this assumption. Tests that relied on this behavior have been changed to no longer rely on this. A test for this situation is added in UpdateNumberOfReplicasIT. --- .../cluster/metadata/MetaData.java | 12 ++++++++---- .../cluster/routing/RoutingTable.java | 12 ++++++++---- .../cluster/routing/RoutingTableTests.java | 2 +- .../allocation/InSyncAllocationIdTests.java | 4 ++-- .../PreferPrimaryAllocationTests.java | 6 ++++-- .../UpdateNumberOfReplicasTests.java | 10 ++++++---- .../settings/UpdateNumberOfReplicasIT.java | 19 +++++++++++++++++++ 7 files changed, 48 insertions(+), 17 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java index 75869b54850d4..26728dc327636 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java @@ -1034,10 +1034,14 @@ public Builder updateSettings(Settings settings, String... indices) { return this; } - public Builder updateNumberOfReplicas(int numberOfReplicas, String... indices) { - if (indices == null || indices.length == 0) { - indices = this.indices.keys().toArray(String.class); - } + /** + * Update the number of replicas for the specified indices. + * + * @param numberOfReplicas the number of replicas + * @param indices the indices to update the number of replicas for + * @return the builder + */ + public Builder updateNumberOfReplicas(int numberOfReplicas, final String[] indices) { for (String index : indices) { IndexMetaData indexMetaData = this.indices.get(index); if (indexMetaData == null) { diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java b/server/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java index 36c512c17aaac..bab150fff12bd 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java +++ b/server/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java @@ -457,13 +457,17 @@ public Builder updateNodes(long version, RoutingNodes routingNodes) { return this; } - public Builder updateNumberOfReplicas(int numberOfReplicas, String... indices) { + /** + * Update the number of replicas for the specified indices. + * + * @param numberOfReplicas the number of replicas + * @param indices the indices to update the number of replicas for + * @return the builder + */ + public Builder updateNumberOfReplicas(final int numberOfReplicas, final String[] indices) { if (indicesRouting == null) { throw new IllegalStateException("once build is called the builder cannot be reused"); } - if (indices == null || indices.length == 0) { - indices = indicesRouting.keys().toArray(String.class); - } for (String index : indices) { IndexRoutingTable indexRoutingTable = indicesRouting.get(index); if (indexRoutingTable == null) { diff --git a/server/src/test/java/org/elasticsearch/cluster/routing/RoutingTableTests.java b/server/src/test/java/org/elasticsearch/cluster/routing/RoutingTableTests.java index 349997d7793eb..0f55270354301 100644 --- a/server/src/test/java/org/elasticsearch/cluster/routing/RoutingTableTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/routing/RoutingTableTests.java @@ -275,7 +275,7 @@ public void testRoutingTableBuiltMoreThanOnce() { assertThat(e.getMessage(), containsString("cannot be reused")); } try { - b.updateNumberOfReplicas(1, "foo"); + b.updateNumberOfReplicas(1, new String[]{"foo"}); fail("expected exception"); } catch (IllegalStateException e) { assertThat(e.getMessage(), containsString("cannot be reused")); diff --git a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/InSyncAllocationIdTests.java b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/InSyncAllocationIdTests.java index 5f39336569fd9..8630d2cc5b6d9 100644 --- a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/InSyncAllocationIdTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/InSyncAllocationIdTests.java @@ -292,8 +292,8 @@ public void testInSyncIdsNotTrimmedWhenNotGrowing() throws Exception { logger.info("decrease number of replicas to 0"); clusterState = ClusterState.builder(clusterState) - .routingTable(RoutingTable.builder(clusterState.routingTable()).updateNumberOfReplicas(0, "test").build()) - .metaData(MetaData.builder(clusterState.metaData()).updateNumberOfReplicas(0, "test")).build(); + .routingTable(RoutingTable.builder(clusterState.routingTable()).updateNumberOfReplicas(0, new String[]{"test"}).build()) + .metaData(MetaData.builder(clusterState.metaData()).updateNumberOfReplicas(0, new String[]{"test"})).build(); logger.info("add back node 1"); clusterState = ClusterState.builder(clusterState).nodes(DiscoveryNodes.builder().add( diff --git a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/PreferPrimaryAllocationTests.java b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/PreferPrimaryAllocationTests.java index d4e032f47614a..35a9be017d5d4 100644 --- a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/PreferPrimaryAllocationTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/PreferPrimaryAllocationTests.java @@ -68,8 +68,10 @@ public void testPreferPrimaryAllocationOverReplicas() { } logger.info("increasing the number of replicas to 1, and perform a reroute (to get the replicas allocation going)"); - RoutingTable updatedRoutingTable = RoutingTable.builder(clusterState.routingTable()).updateNumberOfReplicas(1).build(); - metaData = MetaData.builder(clusterState.metaData()).updateNumberOfReplicas(1).build(); + final String[] indices = {"test1", "test2"}; + RoutingTable updatedRoutingTable = + RoutingTable.builder(clusterState.routingTable()).updateNumberOfReplicas(1, indices).build(); + metaData = MetaData.builder(clusterState.metaData()).updateNumberOfReplicas(1, indices).build(); clusterState = ClusterState.builder(clusterState).routingTable(updatedRoutingTable).metaData(metaData).build(); clusterState = strategy.reroute(clusterState, "reroute"); diff --git a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/UpdateNumberOfReplicasTests.java b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/UpdateNumberOfReplicasTests.java index 167172ec9bd15..3001a4ba9e423 100644 --- a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/UpdateNumberOfReplicasTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/UpdateNumberOfReplicasTests.java @@ -96,8 +96,10 @@ public void testUpdateNumberOfReplicas() { logger.info("add another replica"); routingNodes = clusterState.getRoutingNodes(); - RoutingTable updatedRoutingTable = RoutingTable.builder(clusterState.routingTable()).updateNumberOfReplicas(2).build(); - metaData = MetaData.builder(clusterState.metaData()).updateNumberOfReplicas(2).build(); + final String[] indices = {"test"}; + RoutingTable updatedRoutingTable = + RoutingTable.builder(clusterState.routingTable()).updateNumberOfReplicas(2, indices).build(); + metaData = MetaData.builder(clusterState.metaData()).updateNumberOfReplicas(2, indices).build(); clusterState = ClusterState.builder(clusterState).routingTable(updatedRoutingTable).metaData(metaData).build(); assertThat(clusterState.metaData().index("test").getNumberOfReplicas(), equalTo(2)); @@ -143,8 +145,8 @@ public void testUpdateNumberOfReplicas() { logger.info("now remove a replica"); routingNodes = clusterState.getRoutingNodes(); - updatedRoutingTable = RoutingTable.builder(clusterState.routingTable()).updateNumberOfReplicas(1).build(); - metaData = MetaData.builder(clusterState.metaData()).updateNumberOfReplicas(1).build(); + updatedRoutingTable = RoutingTable.builder(clusterState.routingTable()).updateNumberOfReplicas(1, indices).build(); + metaData = MetaData.builder(clusterState.metaData()).updateNumberOfReplicas(1, indices).build(); clusterState = ClusterState.builder(clusterState).routingTable(updatedRoutingTable).metaData(metaData).build(); assertThat(clusterState.metaData().index("test").getNumberOfReplicas(), equalTo(1)); diff --git a/server/src/test/java/org/elasticsearch/indices/settings/UpdateNumberOfReplicasIT.java b/server/src/test/java/org/elasticsearch/indices/settings/UpdateNumberOfReplicasIT.java index 9b759bff56928..100cda1c03cef 100644 --- a/server/src/test/java/org/elasticsearch/indices/settings/UpdateNumberOfReplicasIT.java +++ b/server/src/test/java/org/elasticsearch/indices/settings/UpdateNumberOfReplicasIT.java @@ -21,6 +21,7 @@ import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse; import org.elasticsearch.action.search.SearchResponse; +import org.elasticsearch.action.support.IndicesOptions; import org.elasticsearch.cluster.health.ClusterHealthStatus; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.common.Priority; @@ -28,6 +29,7 @@ import org.elasticsearch.test.ESIntegTestCase; import java.io.IOException; +import java.util.EnumSet; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery; @@ -274,4 +276,21 @@ public void testUpdateWithInvalidNumberOfReplicas() { assertEquals("Failed to parse value [" + value + "] for setting [index.number_of_replicas] must be >= 0", e.getMessage()); } } + + public void testUpdateNumberOfReplicasAllowNoIndices() { + createIndex("test-index", Settings.builder().put("index.number_of_replicas", 0).build()); + final IndicesOptions options = + new IndicesOptions(EnumSet.of(IndicesOptions.Option.ALLOW_NO_INDICES), EnumSet.of(IndicesOptions.WildcardStates.OPEN)); + assertAcked(client() + .admin() + .indices() + .prepareUpdateSettings("non-existent-*") + .setSettings(Settings.builder().put("index.number_of_replicas", 1)) + .setIndicesOptions(options) + .get()); + final int numberOfReplicas = Integer.parseInt( + client().admin().indices().prepareGetSettings("test-index").get().getSetting("test-index", "index.number_of_replicas")); + assertThat(numberOfReplicas, equalTo(0)); + } + } From 7a2e0c8fa75295f21d45ed0f83e3ce167265fc9b Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 15 Oct 2018 15:16:28 -0400 Subject: [PATCH 2/2] Add a missing final --- .../main/java/org/elasticsearch/cluster/metadata/MetaData.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java index 26728dc327636..f4eafd05e1599 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java @@ -1041,7 +1041,7 @@ public Builder updateSettings(Settings settings, String... indices) { * @param indices the indices to update the number of replicas for * @return the builder */ - public Builder updateNumberOfReplicas(int numberOfReplicas, final String[] indices) { + public Builder updateNumberOfReplicas(final int numberOfReplicas, final String[] indices) { for (String index : indices) { IndexMetaData indexMetaData = this.indices.get(index); if (indexMetaData == null) {