From 3f61f93f6660b32153047b7fb0393b314510d238 Mon Sep 17 00:00:00 2001 From: Henning Andersen Date: Tue, 7 Jul 2020 22:57:38 +0200 Subject: [PATCH] Always call the over limit strategy Now determine strategy based on whether real memory usage is tracked. --- .../HierarchyCircuitBreakerService.java | 21 ++++------- .../HierarchyCircuitBreakerServiceTests.java | 36 +++++++++++++------ 2 files changed, 33 insertions(+), 24 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerService.java b/server/src/main/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerService.java index 21916413b48d8..ce4eba3e8d156 100644 --- a/server/src/main/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerService.java +++ b/server/src/main/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerService.java @@ -45,6 +45,7 @@ import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicLong; +import java.util.function.Function; import java.util.function.LongSupplier; import java.util.stream.Collectors; @@ -113,11 +114,11 @@ public class HierarchyCircuitBreakerService extends CircuitBreakerService { private final OverLimitStrategy overLimitStrategy; public HierarchyCircuitBreakerService(Settings settings, List customBreakers, ClusterSettings clusterSettings) { - this(settings, customBreakers, clusterSettings, createOverLimitStrategy()); + this(settings, customBreakers, clusterSettings, HierarchyCircuitBreakerService::createOverLimitStrategy); } HierarchyCircuitBreakerService(Settings settings, List customBreakers, ClusterSettings clusterSettings, - OverLimitStrategy overLimitStrategy) { + Function overLimitStrategyFactory) { super(); HashMap childCircuitBreakers = new HashMap<>(); childCircuitBreakers.put(CircuitBreaker.FIELDDATA, validateAndCreateBreaker( @@ -182,7 +183,7 @@ public HierarchyCircuitBreakerService(Settings settings, List c (name, updatedValues) -> updateCircuitBreakerSettings(name, updatedValues.v1(), updatedValues.v2()), (s, t) -> {}); - this.overLimitStrategy = overLimitStrategy; + this.overLimitStrategy = overLimitStrategyFactory.apply(this.trackRealMemoryUsage); } private void updateCircuitBreakerSettings(String name, ByteSizeValue newLimit, Double newOverhead) { @@ -309,7 +310,7 @@ public long getParentLimit() { public void checkParentLimit(long newBytesReserved, String label) throws CircuitBreakingException { final MemoryUsage memoryUsed = memoryUsed(newBytesReserved); long parentLimit = this.parentSettings.getLimit(); - if (memoryUsed.totalUsage > parentLimit && doubleCheckMemoryUsed(memoryUsed).totalUsage > parentLimit) { + if (memoryUsed.totalUsage > parentLimit && overLimitStrategy.overLimit(memoryUsed).totalUsage > parentLimit) { this.parentTripCount.incrementAndGet(); final StringBuilder message = new StringBuilder("[parent] Data too large, data for [" + label + "]" + " would be [" + memoryUsed.totalUsage + "/" + new ByteSizeValue(memoryUsed.totalUsage) + "]" + @@ -343,14 +344,6 @@ public void checkParentLimit(long newBytesReserved, String label) throws Circuit } } - MemoryUsage doubleCheckMemoryUsed(MemoryUsage memoryUsed) { - if (this.trackRealMemoryUsage) { - return overLimitStrategy.overLimit(memoryUsed); - } else { - return memoryUsed; - } - } - private CircuitBreaker validateAndCreateBreaker(BreakerSettings breakerSettings) { // Validate the settings validateSettings(new BreakerSettings[] {breakerSettings}); @@ -362,9 +355,9 @@ private CircuitBreaker validateAndCreateBreaker(BreakerSettings breakerSettings) breakerSettings.getName()); } - private static OverLimitStrategy createOverLimitStrategy() { + static OverLimitStrategy createOverLimitStrategy(boolean trackRealMemoryUsage) { JvmInfo jvmInfo = JvmInfo.jvmInfo(); - if (jvmInfo.useG1GC().equals("true") + if (trackRealMemoryUsage && jvmInfo.useG1GC().equals("true") // messing with GC is "dangerous" so we apply an escape hatch. Not intended to be used. && Booleans.parseBoolean(System.getProperty("es.real_memory_circuit_breaker.g1.double_check.enabled"), true)) { // hardcode interval, do not want any tuning of it outside code changes. diff --git a/server/src/test/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerServiceTests.java b/server/src/test/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerServiceTests.java index d531ba87c3163..1368c42184b56 100644 --- a/server/src/test/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerServiceTests.java +++ b/server/src/test/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerServiceTests.java @@ -53,6 +53,7 @@ import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThanOrEqualTo; +import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.lessThanOrEqualTo; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.nullValue; @@ -305,7 +306,8 @@ public void testParentTriggersG1GCBeforeBreaking() throws InterruptedException, final HierarchyCircuitBreakerService service = new HierarchyCircuitBreakerService(clusterSettings, Collections.emptyList(), new ClusterSettings(clusterSettings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), - new HierarchyCircuitBreakerService.G1OverLimitStrategy(JvmInfo.jvmInfo(), HierarchyCircuitBreakerService::realMemoryUsage, + trackRealMemoryUsage -> new HierarchyCircuitBreakerService.G1OverLimitStrategy(JvmInfo.jvmInfo(), + HierarchyCircuitBreakerService::realMemoryUsage, HierarchyCircuitBreakerService.createYoungGcCountSupplier(), time::get, interval) { @Override @@ -383,16 +385,18 @@ public void testParentDoesOverLimitCheck() { final HierarchyCircuitBreakerService service = new HierarchyCircuitBreakerService(clusterSettings, Collections.emptyList(), new ClusterSettings(clusterSettings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS), - memoryUsed -> { - assertTrue(overLimitTriggered.compareAndSet(false, true)); - if (saveTheDay) { - return new HierarchyCircuitBreakerService.MemoryUsage(memoryUsed.baseUsage / 2, - memoryUsed.totalUsage - (memoryUsed.baseUsage / 2), memoryUsed.transientChildUsage, - memoryUsed.permanentChildUsage); - } else { - return memoryUsed; + trackRealMemoryUsage -> + memoryUsed -> { + assertTrue(overLimitTriggered.compareAndSet(false, true)); + if (saveTheDay) { + return new HierarchyCircuitBreakerService.MemoryUsage(memoryUsed.baseUsage / 2, + memoryUsed.totalUsage - (memoryUsed.baseUsage / 2), memoryUsed.transientChildUsage, + memoryUsed.permanentChildUsage); + } else { + return memoryUsed; + } } - }); + ); int allocationSize = g1RegionSize > 0 ? (int) (g1RegionSize / 2) : 1024 * 1024; int allocationCount = (int) (JvmInfo.jvmInfo().getConfiguredMaxHeapSize() / allocationSize) + 1; @@ -558,6 +562,18 @@ void overLimitTriggered(boolean leader) { threads.forEach(thread -> assertFalse(thread.isAlive())); } + public void testCreateOverLimitStrategy() { + assertThat(HierarchyCircuitBreakerService.createOverLimitStrategy(false), + not(instanceOf(HierarchyCircuitBreakerService.G1OverLimitStrategy.class))); + if (JvmInfo.jvmInfo().useG1GC().equals("true")) { + assertThat(HierarchyCircuitBreakerService.createOverLimitStrategy(true), + instanceOf(HierarchyCircuitBreakerService.G1OverLimitStrategy.class)); + } else { + assertThat(HierarchyCircuitBreakerService.createOverLimitStrategy(true), + not(instanceOf(HierarchyCircuitBreakerService.G1OverLimitStrategy.class))); + } + } + public void testTrippedCircuitBreakerDurability() { Settings clusterSettings = Settings.builder() .put(HierarchyCircuitBreakerService.USE_REAL_MEMORY_USAGE_SETTING.getKey(), Boolean.FALSE)