diff --git a/src/main/java/org/elasticsearch/cluster/settings/ClusterDynamicSettingsModule.java b/src/main/java/org/elasticsearch/cluster/settings/ClusterDynamicSettingsModule.java index 7dd301188666b..8701eb8abccce 100644 --- a/src/main/java/org/elasticsearch/cluster/settings/ClusterDynamicSettingsModule.java +++ b/src/main/java/org/elasticsearch/cluster/settings/ClusterDynamicSettingsModule.java @@ -94,7 +94,6 @@ public ClusterDynamicSettingsModule() { clusterDynamicSettings.addDynamicSetting(HierarchyCircuitBreakerService.FIELDDATA_CIRCUIT_BREAKER_OVERHEAD_SETTING, Validator.NON_NEGATIVE_DOUBLE); clusterDynamicSettings.addDynamicSetting(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_LIMIT_SETTING, Validator.MEMORY_SIZE); clusterDynamicSettings.addDynamicSetting(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_OVERHEAD_SETTING, Validator.NON_NEGATIVE_DOUBLE); - clusterDynamicSettings.addDynamicSetting(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_TYPE_SETTING); } public void addDynamicSettings(String... settings) { diff --git a/src/main/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerService.java b/src/main/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerService.java index 8135592c86219..7e52aa30cb8b9 100644 --- a/src/main/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerService.java +++ b/src/main/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerService.java @@ -144,8 +144,6 @@ public class ApplySettings implements NodeSettingsService.Listener { public void onRefreshSettings(Settings settings) { boolean changed = false; - String newRequestType = settings.get(REQUEST_CIRCUIT_BREAKER_TYPE_SETTING, null); - // Fielddata settings BreakerSettings newFielddataSettings = HierarchyCircuitBreakerService.this.fielddataSettings; ByteSizeValue newFielddataMax = settings.getAsMemory(FIELDDATA_CIRCUIT_BREAKER_LIMIT_SETTING, null); @@ -163,13 +161,13 @@ public void onRefreshSettings(Settings settings) { BreakerSettings newRequestSettings = HierarchyCircuitBreakerService.this.requestSettings; ByteSizeValue newRequestMax = settings.getAsMemory(REQUEST_CIRCUIT_BREAKER_LIMIT_SETTING, null); Double newRequestOverhead = settings.getAsDouble(REQUEST_CIRCUIT_BREAKER_OVERHEAD_SETTING, null); - if (newRequestMax != null || newRequestOverhead != null || newRequestType != null) { + if (newRequestMax != null || newRequestOverhead != null) { changed = true; long newRequestLimitBytes = newRequestMax == null ? HierarchyCircuitBreakerService.this.requestSettings.getLimit() : newRequestMax.bytes(); newRequestOverhead = newRequestOverhead == null ? HierarchyCircuitBreakerService.this.requestSettings.getOverhead() : newRequestOverhead; - CircuitBreaker.Type newType = newRequestType == null ? HierarchyCircuitBreakerService.this.requestSettings.getType() : CircuitBreaker.Type.parseValue(newRequestType); - newRequestSettings = new BreakerSettings(CircuitBreaker.Name.REQUEST, newRequestLimitBytes, newRequestOverhead, newType); + newRequestSettings = new BreakerSettings(CircuitBreaker.Name.REQUEST, newRequestLimitBytes, newRequestOverhead, + HierarchyCircuitBreakerService.this.requestSettings.getType()); } // Parent settings @@ -185,8 +183,6 @@ public void onRefreshSettings(Settings settings) { // change all the things validateSettings(new BreakerSettings[]{newFielddataSettings, newRequestSettings}); logger.info("Updating settings parent: {}, fielddata: {}, request: {}", newParentSettings, newFielddataSettings, newRequestSettings); - CircuitBreaker.Type previousFielddataType = HierarchyCircuitBreakerService.this.fielddataSettings.getType(); - CircuitBreaker.Type previousRequestType = HierarchyCircuitBreakerService.this.requestSettings.getType(); HierarchyCircuitBreakerService.this.parentSettings = newParentSettings; HierarchyCircuitBreakerService.this.fielddataSettings = newFielddataSettings; HierarchyCircuitBreakerService.this.requestSettings = newRequestSettings; @@ -196,29 +192,18 @@ public void onRefreshSettings(Settings settings) { if (newFielddataSettings.getType() == CircuitBreaker.Type.NOOP) { fielddataBreaker = new NoopCircuitBreaker(CircuitBreaker.Name.FIELDDATA); } else { - if (previousFielddataType == CircuitBreaker.Type.MEMORY) { - fielddataBreaker = new ChildMemoryCircuitBreaker(newFielddataSettings, - (ChildMemoryCircuitBreaker) HierarchyCircuitBreakerService.this.breakers.get(CircuitBreaker.Name.FIELDDATA), - logger, HierarchyCircuitBreakerService.this, CircuitBreaker.Name.FIELDDATA); - } else { - fielddataBreaker = new ChildMemoryCircuitBreaker(newFielddataSettings, - logger, HierarchyCircuitBreakerService.this, CircuitBreaker.Name.FIELDDATA); - - } + fielddataBreaker = new ChildMemoryCircuitBreaker(newFielddataSettings, + (ChildMemoryCircuitBreaker) HierarchyCircuitBreakerService.this.breakers.get(CircuitBreaker.Name.FIELDDATA), + logger, HierarchyCircuitBreakerService.this, CircuitBreaker.Name.FIELDDATA); } CircuitBreaker requestBreaker; if (newRequestSettings.getType() == CircuitBreaker.Type.NOOP) { requestBreaker = new NoopCircuitBreaker(CircuitBreaker.Name.REQUEST); } else { - if (previousRequestType == CircuitBreaker.Type.MEMORY) { - requestBreaker = new ChildMemoryCircuitBreaker(newRequestSettings, - (ChildMemoryCircuitBreaker)HierarchyCircuitBreakerService.this.breakers.get(CircuitBreaker.Name.REQUEST), - logger, HierarchyCircuitBreakerService.this, CircuitBreaker.Name.REQUEST); - } else { - requestBreaker = new ChildMemoryCircuitBreaker(newRequestSettings, - logger, HierarchyCircuitBreakerService.this, CircuitBreaker.Name.REQUEST); - } + requestBreaker = new ChildMemoryCircuitBreaker(newRequestSettings, + (ChildMemoryCircuitBreaker)HierarchyCircuitBreakerService.this.breakers.get(CircuitBreaker.Name.REQUEST), + logger, HierarchyCircuitBreakerService.this, CircuitBreaker.Name.REQUEST); } tempBreakers.put(CircuitBreaker.Name.FIELDDATA, fielddataBreaker); diff --git a/src/test/java/org/elasticsearch/indices/memory/breaker/CircuitBreakerNoopTests.java b/src/test/java/org/elasticsearch/indices/memory/breaker/CircuitBreakerNoopTests.java new file mode 100644 index 0000000000000..313a8eec70f9d --- /dev/null +++ b/src/test/java/org/elasticsearch/indices/memory/breaker/CircuitBreakerNoopTests.java @@ -0,0 +1,91 @@ +/* + * 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.indices.memory.breaker; + +import org.elasticsearch.action.index.IndexRequestBuilder; +import org.elasticsearch.client.Client; +import org.elasticsearch.common.settings.ImmutableSettings; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.indices.breaker.HierarchyCircuitBreakerService; +import org.elasticsearch.search.sort.SortOrder; +import org.elasticsearch.test.ElasticsearchIntegrationTest; +import org.junit.Test; + +import java.util.List; + +import static com.google.common.collect.Lists.newArrayList; +import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_NUMBER_OF_REPLICAS; +import static org.elasticsearch.common.settings.ImmutableSettings.settingsBuilder; +import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery; +import static org.elasticsearch.search.aggregations.AggregationBuilders.cardinality; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; + +/** Tests for the noop breakers, which are non-dynamic settings */ +@ElasticsearchIntegrationTest.ClusterScope(scope=ElasticsearchIntegrationTest.Scope.SUITE, numDataNodes=0) +public class CircuitBreakerNoopTests extends ElasticsearchIntegrationTest { + + @Override + protected Settings nodeSettings(int nodeOrdinal) { + return ImmutableSettings.builder() + .put(HierarchyCircuitBreakerService.FIELDDATA_CIRCUIT_BREAKER_TYPE_SETTING, "noop") + // This is set low, because if the "noop" is not a noop, it will break + .put(HierarchyCircuitBreakerService.FIELDDATA_CIRCUIT_BREAKER_LIMIT_SETTING, "10b") + .put(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_TYPE_SETTING, "noop") + // This is set low, because if the "noop" is not a noop, it will break + .put(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_LIMIT_SETTING, "10b") + .build(); + } + + @Test + public void testNoopRequestBreaker() throws Exception { + assertAcked(prepareCreate("cb-test", 1, settingsBuilder().put(SETTING_NUMBER_OF_REPLICAS, between(0, 1)))); + Client client = client(); + + // index some different terms so we have some field data for loading + int docCount = scaledRandomIntBetween(300, 1000); + List reqs = newArrayList(); + for (long id = 0; id < docCount; id++) { + reqs.add(client.prepareIndex("cb-test", "type", Long.toString(id)).setSource("test", id)); + } + indexRandom(true, reqs); + + // A cardinality aggregation uses BigArrays and thus the REQUEST breaker + client.prepareSearch("cb-test").setQuery(matchAllQuery()).addAggregation(cardinality("card").field("test")).get(); + // no exception because the breaker is a noop + } + + @Test + public void testNoopFielddataBreaker() throws Exception { + assertAcked(prepareCreate("cb-test", 1, settingsBuilder().put(SETTING_NUMBER_OF_REPLICAS, between(0, 1)))); + Client client = client(); + + // index some different terms so we have some field data for loading + int docCount = scaledRandomIntBetween(300, 1000); + List reqs = newArrayList(); + for (long id = 0; id < docCount; id++) { + reqs.add(client.prepareIndex("cb-test", "type", Long.toString(id)).setSource("test", id)); + } + indexRandom(true, reqs); + + // Sorting using fielddata and thus the FIELDDATA breaker + client.prepareSearch("cb-test").setQuery(matchAllQuery()).addSort("test", SortOrder.DESC).get(); + // no exception because the breaker is a noop + } +} diff --git a/src/test/java/org/elasticsearch/indices/memory/breaker/CircuitBreakerServiceTests.java b/src/test/java/org/elasticsearch/indices/memory/breaker/CircuitBreakerServiceTests.java index 65cf70344efff..bd313eab7cb1a 100644 --- a/src/test/java/org/elasticsearch/indices/memory/breaker/CircuitBreakerServiceTests.java +++ b/src/test/java/org/elasticsearch/indices/memory/breaker/CircuitBreakerServiceTests.java @@ -70,8 +70,6 @@ private void reset() { .put(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_LIMIT_SETTING, HierarchyCircuitBreakerService.DEFAULT_REQUEST_BREAKER_LIMIT) .put(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_OVERHEAD_SETTING, 1.0) - .put(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_TYPE_SETTING, - HierarchyCircuitBreakerService.DEFAULT_BREAKER_TYPE) .build(); client().admin().cluster().prepareUpdateSettings().setTransientSettings(resetSettings).execute().actionGet(); } @@ -90,9 +88,27 @@ private String randomRidiculouslySmallLimit() { return randomFrom(Arrays.asList("100b", "100")); } + /** Returns true if any of the nodes used a noop breaker */ + private boolean noopBreakerUsed() { + NodesStatsResponse stats = client().admin().cluster().prepareNodesStats().setBreaker(true).get(); + for (NodeStats nodeStats : stats) { + if (nodeStats.getBreaker().getStats(CircuitBreaker.Name.REQUEST).getLimit() == 0) { + return true; + } + if (nodeStats.getBreaker().getStats(CircuitBreaker.Name.FIELDDATA).getLimit() == 0) { + return true; + } + } + return false; + } + @Test //@TestLogging("indices.breaker:TRACE,index.fielddata:TRACE,common.breaker:TRACE") public void testMemoryBreaker() throws Exception { + if (noopBreakerUsed()) { + logger.info("--> noop breakers used, skipping test"); + return; + } assertAcked(prepareCreate("cb-test", 1, settingsBuilder().put(SETTING_NUMBER_OF_REPLICAS, between(0, 1)))); final Client client = client(); @@ -134,6 +150,10 @@ public void testMemoryBreaker() throws Exception { @Test public void testRamAccountingTermsEnum() throws Exception { + if (noopBreakerUsed()) { + logger.info("--> noop breakers used, skipping test"); + return; + } final Client client = client(); // Create an index where the mappings have a field data filter @@ -184,6 +204,10 @@ public void testRamAccountingTermsEnum() throws Exception { */ @Test public void testParentChecking() throws Exception { + if (noopBreakerUsed()) { + logger.info("--> noop breakers used, skipping test"); + return; + } assertAcked(prepareCreate("cb-test", 1, settingsBuilder().put(SETTING_NUMBER_OF_REPLICAS, between(0, 1)))); Client client = client(); @@ -240,36 +264,10 @@ public void testParentChecking() throws Exception { @Test public void testRequestBreaker() throws Exception { - assertAcked(prepareCreate("cb-test", 1, settingsBuilder().put(SETTING_NUMBER_OF_REPLICAS, between(0, 1)))); - Client client = client(); - - // Make request breaker limited to a small amount - Settings resetSettings = settingsBuilder() - .put(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_LIMIT_SETTING, "10b") - .build(); - client.admin().cluster().prepareUpdateSettings().setTransientSettings(resetSettings).execute().actionGet(); - - // index some different terms so we have some field data for loading - int docCount = scaledRandomIntBetween(300, 1000); - List reqs = newArrayList(); - for (long id = 0; id < docCount; id++) { - reqs.add(client.prepareIndex("cb-test", "type", Long.toString(id)).setSource("test", id)); + if (noopBreakerUsed()) { + logger.info("--> noop breakers used, skipping test"); + return; } - indexRandom(true, reqs); - - // A cardinality aggregation uses BigArrays and thus the REQUEST breaker - try { - client.prepareSearch("cb-test").setQuery(matchAllQuery()).addAggregation(cardinality("card").field("test")).get(); - fail("aggregation should have tripped the breaker"); - } catch (Exception e) { - String errMsg = "CircuitBreakingException[[REQUEST] Data too large, data for [] would be larger than limit of [10/10b]]"; - assertThat("Exception: " + ExceptionsHelper.unwrapCause(e) + " should contain a CircuitBreakingException", - ExceptionsHelper.unwrapCause(e).getMessage().contains(errMsg), equalTo(true)); - } - } - - @Test - public void testNoopRequestBreaker() throws Exception { assertAcked(prepareCreate("cb-test", 1, settingsBuilder().put(SETTING_NUMBER_OF_REPLICAS, between(0, 1)))); Client client = client(); @@ -296,14 +294,5 @@ public void testNoopRequestBreaker() throws Exception { assertThat("Exception: " + ExceptionsHelper.unwrapCause(e) + " should contain a CircuitBreakingException", ExceptionsHelper.unwrapCause(e).getMessage().contains(errMsg), equalTo(true)); } - - // Make request breaker into a noop breaker - resetSettings = settingsBuilder() - .put(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_TYPE_SETTING, "noop") - .build(); - client.admin().cluster().prepareUpdateSettings().setTransientSettings(resetSettings).execute().actionGet(); - - // A cardinality aggregation uses BigArrays and thus the REQUEST breaker - client.prepareSearch("cb-test").setQuery(matchAllQuery()).addAggregation(cardinality("card").field("test")).get(); } } diff --git a/src/test/java/org/elasticsearch/test/ElasticsearchIntegrationTest.java b/src/test/java/org/elasticsearch/test/ElasticsearchIntegrationTest.java index 0486a8ee89c96..1f3dcb725bc37 100644 --- a/src/test/java/org/elasticsearch/test/ElasticsearchIntegrationTest.java +++ b/src/test/java/org/elasticsearch/test/ElasticsearchIntegrationTest.java @@ -60,7 +60,6 @@ import org.elasticsearch.common.Nullable; import org.elasticsearch.common.Priority; import org.elasticsearch.common.Strings; -import org.elasticsearch.common.breaker.CircuitBreaker; import org.elasticsearch.common.collect.ImmutableOpenMap; import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.settings.ImmutableSettings; @@ -91,7 +90,6 @@ import org.elasticsearch.index.translog.fs.FsTranslog; import org.elasticsearch.index.translog.fs.FsTranslogFile; import org.elasticsearch.indices.IndicesService; -import org.elasticsearch.indices.breaker.HierarchyCircuitBreakerService; import org.elasticsearch.indices.cache.query.IndicesQueryCache; import org.elasticsearch.indices.recovery.RecoverySettings; import org.elasticsearch.indices.store.IndicesStore; @@ -450,20 +448,11 @@ protected boolean randomizeNumberOfShardsAndReplicas() { return compatibilityVersion().onOrAfter(Version.V_1_1_0); } - /** Rarely set the request breaker to a Noop breaker */ - protected static void setRandomBreakerSettings(Random random, ImmutableSettings.Builder builder) { - // Rarely - if (RandomInts.randomInt(random, 100) >= 90) { - builder.put(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_TYPE_SETTING, CircuitBreaker.Type.NOOP); - } - } - private static ImmutableSettings.Builder setRandomSettings(Random random, ImmutableSettings.Builder builder) { setRandomMerge(random, builder); setRandomTranslogSettings(random, builder); setRandomNormsLoading(random, builder); setRandomScriptingSettings(random, builder); - setRandomBreakerSettings(random, builder); if (random.nextBoolean()) { if (random.nextInt(10) == 0) { // do something crazy slow here builder.put(IndicesStore.INDICES_STORE_THROTTLE_MAX_BYTES_PER_SEC, new ByteSizeValue(RandomInts.randomIntBetween(random, 1, 10), ByteSizeUnit.MB)); diff --git a/src/test/java/org/elasticsearch/test/InternalTestCluster.java b/src/test/java/org/elasticsearch/test/InternalTestCluster.java index f7b959abae704..1b9f0ab07f595 100644 --- a/src/test/java/org/elasticsearch/test/InternalTestCluster.java +++ b/src/test/java/org/elasticsearch/test/InternalTestCluster.java @@ -68,8 +68,8 @@ import org.elasticsearch.index.cache.filter.none.NoneFilterCache; import org.elasticsearch.index.cache.filter.weighted.WeightedFilterCache; import org.elasticsearch.index.engine.IndexEngineModule; -import org.elasticsearch.index.mapper.MapperService; import org.elasticsearch.indices.breaker.CircuitBreakerService; +import org.elasticsearch.indices.breaker.HierarchyCircuitBreakerService; import org.elasticsearch.node.Node; import org.elasticsearch.node.internal.InternalNode; import org.elasticsearch.node.service.NodeService; @@ -401,6 +401,11 @@ private static Settings getRandomNodeSettings(long seed) { builder.put(MappingUpdatedAction.INDICES_MAPPING_ADDITIONAL_MAPPING_CHANGE_TIME, RandomInts.randomIntBetween(random, 0, 500) /*milliseconds*/); } + if (random.nextInt(10) == 0) { + builder.put(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_TYPE_SETTING, "noop"); + builder.put(HierarchyCircuitBreakerService.FIELDDATA_CIRCUIT_BREAKER_TYPE_SETTING, "noop"); + } + return builder.build(); }