Skip to content

Commit abfaada

Browse files
dakronedakrone
committed
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
1 parent fa3fde2 commit abfaada

File tree

6 files changed

+135
-76
lines changed

6 files changed

+135
-76
lines changed

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

-1
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,6 @@ public ClusterDynamicSettingsModule() {
9393
clusterDynamicSettings.addDynamicSetting(HierarchyCircuitBreakerService.FIELDDATA_CIRCUIT_BREAKER_OVERHEAD_SETTING, Validator.NON_NEGATIVE_DOUBLE);
9494
clusterDynamicSettings.addDynamicSetting(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_LIMIT_SETTING, Validator.MEMORY_SIZE);
9595
clusterDynamicSettings.addDynamicSetting(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_OVERHEAD_SETTING, Validator.NON_NEGATIVE_DOUBLE);
96-
clusterDynamicSettings.addDynamicSetting(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_TYPE_SETTING);
9796
}
9897

9998
public void addDynamicSettings(String... settings) {

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

+9-24
Original file line numberDiff line numberDiff line change
@@ -144,8 +144,6 @@ public class ApplySettings implements NodeSettingsService.Listener {
144144
public void onRefreshSettings(Settings settings) {
145145
boolean changed = false;
146146

147-
String newRequestType = settings.get(REQUEST_CIRCUIT_BREAKER_TYPE_SETTING, null);
148-
149147
// Fielddata settings
150148
BreakerSettings newFielddataSettings = HierarchyCircuitBreakerService.this.fielddataSettings;
151149
ByteSizeValue newFielddataMax = settings.getAsMemory(FIELDDATA_CIRCUIT_BREAKER_LIMIT_SETTING, null);
@@ -163,13 +161,13 @@ public void onRefreshSettings(Settings settings) {
163161
BreakerSettings newRequestSettings = HierarchyCircuitBreakerService.this.requestSettings;
164162
ByteSizeValue newRequestMax = settings.getAsMemory(REQUEST_CIRCUIT_BREAKER_LIMIT_SETTING, null);
165163
Double newRequestOverhead = settings.getAsDouble(REQUEST_CIRCUIT_BREAKER_OVERHEAD_SETTING, null);
166-
if (newRequestMax != null || newRequestOverhead != null || newRequestType != null) {
164+
if (newRequestMax != null || newRequestOverhead != null) {
167165
changed = true;
168166
long newRequestLimitBytes = newRequestMax == null ? HierarchyCircuitBreakerService.this.requestSettings.getLimit() : newRequestMax.bytes();
169167
newRequestOverhead = newRequestOverhead == null ? HierarchyCircuitBreakerService.this.requestSettings.getOverhead() : newRequestOverhead;
170-
CircuitBreaker.Type newType = newRequestType == null ? HierarchyCircuitBreakerService.this.requestSettings.getType() : CircuitBreaker.Type.parseValue(newRequestType);
171168

172-
newRequestSettings = new BreakerSettings(CircuitBreaker.Name.REQUEST, newRequestLimitBytes, newRequestOverhead, newType);
169+
newRequestSettings = new BreakerSettings(CircuitBreaker.Name.REQUEST, newRequestLimitBytes, newRequestOverhead,
170+
HierarchyCircuitBreakerService.this.requestSettings.getType());
173171
}
174172

175173
// Parent settings
@@ -185,8 +183,6 @@ public void onRefreshSettings(Settings settings) {
185183
// change all the things
186184
validateSettings(new BreakerSettings[]{newFielddataSettings, newRequestSettings});
187185
logger.info("Updating settings parent: {}, fielddata: {}, request: {}", newParentSettings, newFielddataSettings, newRequestSettings);
188-
CircuitBreaker.Type previousFielddataType = HierarchyCircuitBreakerService.this.fielddataSettings.getType();
189-
CircuitBreaker.Type previousRequestType = HierarchyCircuitBreakerService.this.requestSettings.getType();
190186
HierarchyCircuitBreakerService.this.parentSettings = newParentSettings;
191187
HierarchyCircuitBreakerService.this.fielddataSettings = newFielddataSettings;
192188
HierarchyCircuitBreakerService.this.requestSettings = newRequestSettings;
@@ -196,29 +192,18 @@ public void onRefreshSettings(Settings settings) {
196192
if (newFielddataSettings.getType() == CircuitBreaker.Type.NOOP) {
197193
fielddataBreaker = new NoopCircuitBreaker(CircuitBreaker.Name.FIELDDATA);
198194
} else {
199-
if (previousFielddataType == CircuitBreaker.Type.MEMORY) {
200-
fielddataBreaker = new ChildMemoryCircuitBreaker(newFielddataSettings,
201-
(ChildMemoryCircuitBreaker) HierarchyCircuitBreakerService.this.breakers.get(CircuitBreaker.Name.FIELDDATA),
202-
logger, HierarchyCircuitBreakerService.this, CircuitBreaker.Name.FIELDDATA);
203-
} else {
204-
fielddataBreaker = new ChildMemoryCircuitBreaker(newFielddataSettings,
205-
logger, HierarchyCircuitBreakerService.this, CircuitBreaker.Name.FIELDDATA);
206-
207-
}
195+
fielddataBreaker = new ChildMemoryCircuitBreaker(newFielddataSettings,
196+
(ChildMemoryCircuitBreaker) HierarchyCircuitBreakerService.this.breakers.get(CircuitBreaker.Name.FIELDDATA),
197+
logger, HierarchyCircuitBreakerService.this, CircuitBreaker.Name.FIELDDATA);
208198
}
209199

210200
CircuitBreaker requestBreaker;
211201
if (newRequestSettings.getType() == CircuitBreaker.Type.NOOP) {
212202
requestBreaker = new NoopCircuitBreaker(CircuitBreaker.Name.REQUEST);
213203
} else {
214-
if (previousRequestType == CircuitBreaker.Type.MEMORY) {
215-
requestBreaker = new ChildMemoryCircuitBreaker(newRequestSettings,
216-
(ChildMemoryCircuitBreaker)HierarchyCircuitBreakerService.this.breakers.get(CircuitBreaker.Name.REQUEST),
217-
logger, HierarchyCircuitBreakerService.this, CircuitBreaker.Name.REQUEST);
218-
} else {
219-
requestBreaker = new ChildMemoryCircuitBreaker(newRequestSettings,
220-
logger, HierarchyCircuitBreakerService.this, CircuitBreaker.Name.REQUEST);
221-
}
204+
requestBreaker = new ChildMemoryCircuitBreaker(newRequestSettings,
205+
(ChildMemoryCircuitBreaker)HierarchyCircuitBreakerService.this.breakers.get(CircuitBreaker.Name.REQUEST),
206+
logger, HierarchyCircuitBreakerService.this, CircuitBreaker.Name.REQUEST);
222207
}
223208

224209
tempBreakers.put(CircuitBreaker.Name.FIELDDATA, fielddataBreaker);

‎src/test/java/org/elasticsearch/indices/memory/breaker/CircuitBreakerNoopTests.java

+91
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
/*
2+
* Licensed to Elasticsearch under one or more contributor
3+
* license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright
5+
* ownership. Elasticsearch licenses this file to you under
6+
* the Apache License, Version 2.0 (the "License"); you may
7+
* not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package org.elasticsearch.indices.memory.breaker;
21+
22+
import org.elasticsearch.action.index.IndexRequestBuilder;
23+
import org.elasticsearch.client.Client;
24+
import org.elasticsearch.common.settings.ImmutableSettings;
25+
import org.elasticsearch.common.settings.Settings;
26+
import org.elasticsearch.indices.breaker.HierarchyCircuitBreakerService;
27+
import org.elasticsearch.search.sort.SortOrder;
28+
import org.elasticsearch.test.ElasticsearchIntegrationTest;
29+
import org.junit.Test;
30+
31+
import java.util.List;
32+
33+
import static com.google.common.collect.Lists.newArrayList;
34+
import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_NUMBER_OF_REPLICAS;
35+
import static org.elasticsearch.common.settings.ImmutableSettings.settingsBuilder;
36+
import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery;
37+
import static org.elasticsearch.search.aggregations.AggregationBuilders.cardinality;
38+
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
39+
40+
/** Tests for the noop breakers, which are non-dynamic settings */
41+
@ElasticsearchIntegrationTest.ClusterScope(scope=ElasticsearchIntegrationTest.Scope.SUITE, numDataNodes=0)
42+
public class CircuitBreakerNoopTests extends ElasticsearchIntegrationTest {
43+
44+
@Override
45+
protected Settings nodeSettings(int nodeOrdinal) {
46+
return ImmutableSettings.builder()
47+
.put(HierarchyCircuitBreakerService.FIELDDATA_CIRCUIT_BREAKER_TYPE_SETTING, "noop")
48+
// This is set low, because if the "noop" is not a noop, it will break
49+
.put(HierarchyCircuitBreakerService.FIELDDATA_CIRCUIT_BREAKER_LIMIT_SETTING, "10b")
50+
.put(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_TYPE_SETTING, "noop")
51+
// This is set low, because if the "noop" is not a noop, it will break
52+
.put(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_LIMIT_SETTING, "10b")
53+
.build();
54+
}
55+
56+
@Test
57+
public void testNoopRequestBreaker() throws Exception {
58+
assertAcked(prepareCreate("cb-test", 1, settingsBuilder().put(SETTING_NUMBER_OF_REPLICAS, between(0, 1))));
59+
Client client = client();
60+
61+
// index some different terms so we have some field data for loading
62+
int docCount = scaledRandomIntBetween(300, 1000);
63+
List<IndexRequestBuilder> reqs = newArrayList();
64+
for (long id = 0; id < docCount; id++) {
65+
reqs.add(client.prepareIndex("cb-test", "type", Long.toString(id)).setSource("test", id));
66+
}
67+
indexRandom(true, reqs);
68+
69+
// A cardinality aggregation uses BigArrays and thus the REQUEST breaker
70+
client.prepareSearch("cb-test").setQuery(matchAllQuery()).addAggregation(cardinality("card").field("test")).get();
71+
// no exception because the breaker is a noop
72+
}
73+
74+
@Test
75+
public void testNoopFielddataBreaker() throws Exception {
76+
assertAcked(prepareCreate("cb-test", 1, settingsBuilder().put(SETTING_NUMBER_OF_REPLICAS, between(0, 1))));
77+
Client client = client();
78+
79+
// index some different terms so we have some field data for loading
80+
int docCount = scaledRandomIntBetween(300, 1000);
81+
List<IndexRequestBuilder> reqs = newArrayList();
82+
for (long id = 0; id < docCount; id++) {
83+
reqs.add(client.prepareIndex("cb-test", "type", Long.toString(id)).setSource("test", id));
84+
}
85+
indexRandom(true, reqs);
86+
87+
// Sorting using fielddata and thus the FIELDDATA breaker
88+
client.prepareSearch("cb-test").setQuery(matchAllQuery()).addSort("test", SortOrder.DESC).get();
89+
// no exception because the breaker is a noop
90+
}
91+
}

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

+29-40
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,6 @@ private void reset() {
7070
.put(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_LIMIT_SETTING,
7171
HierarchyCircuitBreakerService.DEFAULT_REQUEST_BREAKER_LIMIT)
7272
.put(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_OVERHEAD_SETTING, 1.0)
73-
.put(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_TYPE_SETTING,
74-
HierarchyCircuitBreakerService.DEFAULT_BREAKER_TYPE)
7573
.build();
7674
client().admin().cluster().prepareUpdateSettings().setTransientSettings(resetSettings).execute().actionGet();
7775
}
@@ -90,9 +88,27 @@ private String randomRidiculouslySmallLimit() {
9088
return randomFrom(Arrays.asList("100b", "100"));
9189
}
9290

91+
/** Returns true if any of the nodes used a noop breaker */
92+
private boolean noopBreakerUsed() {
93+
NodesStatsResponse stats = client().admin().cluster().prepareNodesStats().setBreaker(true).get();
94+
for (NodeStats nodeStats : stats) {
95+
if (nodeStats.getBreaker().getStats(CircuitBreaker.Name.REQUEST).getLimit() == 0) {
96+
return true;
97+
}
98+
if (nodeStats.getBreaker().getStats(CircuitBreaker.Name.FIELDDATA).getLimit() == 0) {
99+
return true;
100+
}
101+
}
102+
return false;
103+
}
104+
93105
@Test
94106
//@TestLogging("indices.breaker:TRACE,index.fielddata:TRACE,common.breaker:TRACE")
95107
public void testMemoryBreaker() throws Exception {
108+
if (noopBreakerUsed()) {
109+
logger.info("--> noop breakers used, skipping test");
110+
return;
111+
}
96112
assertAcked(prepareCreate("cb-test", 1, settingsBuilder().put(SETTING_NUMBER_OF_REPLICAS, between(0, 1))));
97113
final Client client = client();
98114

@@ -134,6 +150,10 @@ public void testMemoryBreaker() throws Exception {
134150

135151
@Test
136152
public void testRamAccountingTermsEnum() throws Exception {
153+
if (noopBreakerUsed()) {
154+
logger.info("--> noop breakers used, skipping test");
155+
return;
156+
}
137157
final Client client = client();
138158

139159
// Create an index where the mappings have a field data filter
@@ -184,6 +204,10 @@ public void testRamAccountingTermsEnum() throws Exception {
184204
*/
185205
@Test
186206
public void testParentChecking() throws Exception {
207+
if (noopBreakerUsed()) {
208+
logger.info("--> noop breakers used, skipping test");
209+
return;
210+
}
187211
assertAcked(prepareCreate("cb-test", 1, settingsBuilder().put(SETTING_NUMBER_OF_REPLICAS, between(0, 1))));
188212
Client client = client();
189213

@@ -240,36 +264,10 @@ public void testParentChecking() throws Exception {
240264

241265
@Test
242266
public void testRequestBreaker() throws Exception {
243-
assertAcked(prepareCreate("cb-test", 1, settingsBuilder().put(SETTING_NUMBER_OF_REPLICAS, between(0, 1))));
244-
Client client = client();
245-
246-
// Make request breaker limited to a small amount
247-
Settings resetSettings = settingsBuilder()
248-
.put(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_LIMIT_SETTING, "10b")
249-
.build();
250-
client.admin().cluster().prepareUpdateSettings().setTransientSettings(resetSettings).execute().actionGet();
251-
252-
// index some different terms so we have some field data for loading
253-
int docCount = scaledRandomIntBetween(300, 1000);
254-
List<IndexRequestBuilder> reqs = newArrayList();
255-
for (long id = 0; id < docCount; id++) {
256-
reqs.add(client.prepareIndex("cb-test", "type", Long.toString(id)).setSource("test", id));
267+
if (noopBreakerUsed()) {
268+
logger.info("--> noop breakers used, skipping test");
269+
return;
257270
}
258-
indexRandom(true, reqs);
259-
260-
// A cardinality aggregation uses BigArrays and thus the REQUEST breaker
261-
try {
262-
client.prepareSearch("cb-test").setQuery(matchAllQuery()).addAggregation(cardinality("card").field("test")).get();
263-
fail("aggregation should have tripped the breaker");
264-
} catch (Exception e) {
265-
String errMsg = "CircuitBreakingException[[REQUEST] Data too large, data for [<reused_arrays>] would be larger than limit of [10/10b]]";
266-
assertThat("Exception: " + ExceptionsHelper.unwrapCause(e) + " should contain a CircuitBreakingException",
267-
ExceptionsHelper.unwrapCause(e).getMessage().contains(errMsg), equalTo(true));
268-
}
269-
}
270-
271-
@Test
272-
public void testNoopRequestBreaker() throws Exception {
273271
assertAcked(prepareCreate("cb-test", 1, settingsBuilder().put(SETTING_NUMBER_OF_REPLICAS, between(0, 1))));
274272
Client client = client();
275273

@@ -296,14 +294,5 @@ public void testNoopRequestBreaker() throws Exception {
296294
assertThat("Exception: " + ExceptionsHelper.unwrapCause(e) + " should contain a CircuitBreakingException",
297295
ExceptionsHelper.unwrapCause(e).getMessage().contains(errMsg), equalTo(true));
298296
}
299-
300-
// Make request breaker into a noop breaker
301-
resetSettings = settingsBuilder()
302-
.put(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_TYPE_SETTING, "noop")
303-
.build();
304-
client.admin().cluster().prepareUpdateSettings().setTransientSettings(resetSettings).execute().actionGet();
305-
306-
// A cardinality aggregation uses BigArrays and thus the REQUEST breaker
307-
client.prepareSearch("cb-test").setQuery(matchAllQuery()).addAggregation(cardinality("card").field("test")).get();
308297
}
309298
}

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

-11
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,6 @@
6060
import org.elasticsearch.common.Nullable;
6161
import org.elasticsearch.common.Priority;
6262
import org.elasticsearch.common.Strings;
63-
import org.elasticsearch.common.breaker.CircuitBreaker;
6463
import org.elasticsearch.common.collect.ImmutableOpenMap;
6564
import org.elasticsearch.common.collect.Tuple;
6665
import org.elasticsearch.common.settings.ImmutableSettings;
@@ -92,7 +91,6 @@
9291
import org.elasticsearch.index.translog.fs.FsTranslog;
9392
import org.elasticsearch.index.translog.fs.FsTranslogFile;
9493
import org.elasticsearch.indices.IndicesService;
95-
import org.elasticsearch.indices.breaker.HierarchyCircuitBreakerService;
9694
import org.elasticsearch.indices.cache.query.IndicesQueryCache;
9795
import org.elasticsearch.indices.recovery.RecoverySettings;
9896
import org.elasticsearch.indices.store.IndicesStore;
@@ -450,20 +448,11 @@ protected boolean randomizeNumberOfShardsAndReplicas() {
450448
return compatibilityVersion().onOrAfter(Version.V_1_1_0);
451449
}
452450

453-
/** Rarely set the request breaker to a Noop breaker */
454-
protected static void setRandomBreakerSettings(Random random, ImmutableSettings.Builder builder) {
455-
// Rarely
456-
if (RandomInts.randomInt(random, 100) >= 90) {
457-
builder.put(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_TYPE_SETTING, CircuitBreaker.Type.NOOP);
458-
}
459-
}
460-
461451
private static ImmutableSettings.Builder setRandomSettings(Random random, ImmutableSettings.Builder builder) {
462452
setRandomMerge(random, builder);
463453
setRandomTranslogSettings(random, builder);
464454
setRandomNormsLoading(random, builder);
465455
setRandomScriptingSettings(random, builder);
466-
setRandomBreakerSettings(random, builder);
467456
if (random.nextBoolean()) {
468457
if (random.nextInt(10) == 0) { // do something crazy slow here
469458
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

+6
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@
7070
import org.elasticsearch.index.engine.IndexEngineModule;
7171
import org.elasticsearch.index.mapper.MapperService;
7272
import org.elasticsearch.indices.breaker.CircuitBreakerService;
73+
import org.elasticsearch.indices.breaker.HierarchyCircuitBreakerService;
7374
import org.elasticsearch.node.Node;
7475
import org.elasticsearch.node.internal.InternalNode;
7576
import org.elasticsearch.node.service.NodeService;
@@ -405,6 +406,11 @@ private static Settings getRandomNodeSettings(long seed) {
405406
builder.put(MapperService.FIELD_MAPPERS_COLLECTION_SWITCH, RandomInts.randomIntBetween(random, 0, 5));
406407
}
407408

409+
if (random.nextInt(10) == 0) {
410+
builder.put(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_TYPE_SETTING, "noop");
411+
builder.put(HierarchyCircuitBreakerService.FIELDDATA_CIRCUIT_BREAKER_TYPE_SETTING, "noop");
412+
}
413+
408414
return builder.build();
409415
}
410416

0 commit comments

Comments
 (0)