Make "noop" request breaker a non-dynamic setting

The issue with making it dynamic is that in the event a cluster is
switched from a noop to a concrete implementation, there may be
in-flight requests, once these requests complete we adjust the breaker
with a negative number and trip an assertion.

This also rarely uses noop breakers in InternalTestCluster

src/main/java/org/elasticsearch/cluster/settings/ClusterDynamicSettingsModule.java

@@ -94,7 +94,6 @@ public class ClusterDynamicSettingsModule extends AbstractModule {
         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) {

src/main/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerService.java

@@ -144,8 +144,6 @@ public class HierarchyCircuitBreakerService extends CircuitBreakerService {
         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 class HierarchyCircuitBreakerService extends CircuitBreakerService {
             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 class HierarchyCircuitBreakerService extends CircuitBreakerService {
                 // 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 class HierarchyCircuitBreakerService extends CircuitBreakerService {
                 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);
-
-                    }
                 }
 
                 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);
-                    }
                 }
 
                 tempBreakers.put(CircuitBreaker.Name.FIELDDATA, fielddataBreaker);

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<IndexRequestBuilder> 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<IndexRequestBuilder> 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
+    }
+}

src/test/java/org/elasticsearch/indices/memory/breaker/CircuitBreakerServiceTests.java

@@ -70,8 +70,6 @@ public class CircuitBreakerServiceTests extends ElasticsearchIntegrationTest {
                 .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 @@ public class CircuitBreakerServiceTests extends ElasticsearchIntegrationTest {
         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 class CircuitBreakerServiceTests extends ElasticsearchIntegrationTest {
 
     @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 class CircuitBreakerServiceTests extends ElasticsearchIntegrationTest {
      */
     @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,6 +264,10 @@ public class CircuitBreakerServiceTests extends ElasticsearchIntegrationTest {
 
     @Test
     public void testRequestBreaker() 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();
 
@@ -267,43 +295,4 @@ public class CircuitBreakerServiceTests extends ElasticsearchIntegrationTest {
                     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();
-
-        // 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<IndexRequestBuilder> 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
-        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 [<reused_arrays>] 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));
-        }
-
-        // 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();
-    }
 }

src/test/java/org/elasticsearch/test/ElasticsearchIntegrationTest.java

@@ -60,7 +60,6 @@ import org.elasticsearch.cluster.metadata.MetaData;
 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.TranslogService;
 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 @@ public abstract class ElasticsearchIntegrationTest extends ElasticsearchTestCase
         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));

src/test/java/org/elasticsearch/test/InternalTestCluster.java

@@ -68,8 +68,8 @@ import org.elasticsearch.index.cache.filter.FilterCacheModule;
 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 @@ public final class InternalTestCluster extends TestCluster {
             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();
     }