Skip to content

Commit

Permalink
Make "noop" request breaker a non-dynamic setting
Browse files Browse the repository at this point in the history
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
  • Loading branch information
dakrone committed Oct 22, 2014
1 parent 26bc940 commit f7d227e
Show file tree
Hide file tree
Showing 6 changed files with 135 additions and 77 deletions.
Expand Up @@ -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) {
Expand Down
Expand Up @@ -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);
Expand All @@ -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
Expand All @@ -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;
Expand All @@ -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);
Expand Down
@@ -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
}
}
Expand Up @@ -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();
}
Expand All @@ -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();

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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<IndexRequestBuilder> 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 [<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));
}
}

@Test
public void testNoopRequestBreaker() throws Exception {
assertAcked(prepareCreate("cb-test", 1, settingsBuilder().put(SETTING_NUMBER_OF_REPLICAS, between(0, 1))));
Client client = client();

Expand All @@ -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();
}
}
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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));
Expand Down
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}

Expand Down

0 comments on commit f7d227e

Please sign in to comment.