From 8288c9010c4fda54e391b3c6f4fdad662c71e7c3 Mon Sep 17 00:00:00 2001 From: Nick Knize Date: Fri, 4 Feb 2022 19:46:27 -0600 Subject: [PATCH] [Remove] CircuitBreaker Accounting (#2056) RAM Accounting of segments is removed so remove the associated Accounting circuit breaker. Signed-off-by: Nicholas Walter Knize --- .../breaker/CircuitBreakerServiceIT.java | 2 - .../common/breaker/CircuitBreaker.java | 6 --- .../common/settings/ClusterSettings.java | 2 - .../HierarchyCircuitBreakerService.java | 37 ------------------- .../HierarchyCircuitBreakerServiceTests.java | 4 +- .../index/engine/EngineTestCase.java | 3 -- .../opensearch/test/InternalTestCluster.java | 13 ------- 7 files changed, 2 insertions(+), 65 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/memory/breaker/CircuitBreakerServiceIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/memory/breaker/CircuitBreakerServiceIT.java index 2c72379e6b7..b4f53016818 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/memory/breaker/CircuitBreakerServiceIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/memory/breaker/CircuitBreakerServiceIT.java @@ -102,8 +102,6 @@ public class CircuitBreakerServiceIT extends OpenSearchIntegTestCase { HierarchyCircuitBreakerService.FIELDDATA_CIRCUIT_BREAKER_OVERHEAD_SETTING, HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_LIMIT_SETTING, HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_OVERHEAD_SETTING, - HierarchyCircuitBreakerService.ACCOUNTING_CIRCUIT_BREAKER_LIMIT_SETTING, - HierarchyCircuitBreakerService.ACCOUNTING_CIRCUIT_BREAKER_OVERHEAD_SETTING, HierarchyCircuitBreakerService.IN_FLIGHT_REQUESTS_CIRCUIT_BREAKER_LIMIT_SETTING, HierarchyCircuitBreakerService.IN_FLIGHT_REQUESTS_CIRCUIT_BREAKER_OVERHEAD_SETTING, HierarchyCircuitBreakerService.TOTAL_CIRCUIT_BREAKER_LIMIT_SETTING diff --git a/server/src/main/java/org/opensearch/common/breaker/CircuitBreaker.java b/server/src/main/java/org/opensearch/common/breaker/CircuitBreaker.java index 4a2663710c5..ca6c5aa9698 100644 --- a/server/src/main/java/org/opensearch/common/breaker/CircuitBreaker.java +++ b/server/src/main/java/org/opensearch/common/breaker/CircuitBreaker.java @@ -66,12 +66,6 @@ public interface CircuitBreaker { * writing requests on the network layer. */ String IN_FLIGHT_REQUESTS = "in_flight_requests"; - /** - * The accounting breaker tracks things held in memory that is independent - * of the request lifecycle. This includes memory used by Lucene for - * segments. - */ - String ACCOUNTING = "accounting"; enum Type { // A regular or ChildMemoryCircuitBreaker diff --git a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java index 178ce413bd2..308ab13a8a7 100644 --- a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java @@ -329,8 +329,6 @@ public final class ClusterSettings extends AbstractScopedSettings { HierarchyCircuitBreakerService.IN_FLIGHT_REQUESTS_CIRCUIT_BREAKER_OVERHEAD_SETTING, HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_LIMIT_SETTING, HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_OVERHEAD_SETTING, - HierarchyCircuitBreakerService.ACCOUNTING_CIRCUIT_BREAKER_LIMIT_SETTING, - HierarchyCircuitBreakerService.ACCOUNTING_CIRCUIT_BREAKER_OVERHEAD_SETTING, IndexModule.NODE_STORE_ALLOW_MMAP, ClusterApplierService.CLUSTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING, ClusterService.USER_DEFINED_METADATA, diff --git a/server/src/main/java/org/opensearch/indices/breaker/HierarchyCircuitBreakerService.java b/server/src/main/java/org/opensearch/indices/breaker/HierarchyCircuitBreakerService.java index 52b8414159c..ec5d1edf3c4 100644 --- a/server/src/main/java/org/opensearch/indices/breaker/HierarchyCircuitBreakerService.java +++ b/server/src/main/java/org/opensearch/indices/breaker/HierarchyCircuitBreakerService.java @@ -140,26 +140,6 @@ public class HierarchyCircuitBreakerService extends CircuitBreakerService { Property.NodeScope ); - public static final Setting ACCOUNTING_CIRCUIT_BREAKER_LIMIT_SETTING = Setting.memorySizeSetting( - "indices.breaker.accounting.limit", - "100%", - Property.Dynamic, - Property.NodeScope - ); - public static final Setting ACCOUNTING_CIRCUIT_BREAKER_OVERHEAD_SETTING = Setting.doubleSetting( - "indices.breaker.accounting.overhead", - 1.0d, - 0.0d, - Property.Dynamic, - Property.NodeScope - ); - public static final Setting ACCOUNTING_CIRCUIT_BREAKER_TYPE_SETTING = new Setting<>( - "indices.breaker.accounting.type", - "memory", - CircuitBreaker.Type::parseValue, - Property.NodeScope - ); - public static final Setting IN_FLIGHT_REQUESTS_CIRCUIT_BREAKER_LIMIT_SETTING = Setting.memorySizeSetting( "network.breaker.inflight_requests.limit", "100%", @@ -236,18 +216,6 @@ public class HierarchyCircuitBreakerService extends CircuitBreakerService { ) ) ); - childCircuitBreakers.put( - CircuitBreaker.ACCOUNTING, - validateAndCreateBreaker( - new BreakerSettings( - CircuitBreaker.ACCOUNTING, - ACCOUNTING_CIRCUIT_BREAKER_LIMIT_SETTING.get(settings).getBytes(), - ACCOUNTING_CIRCUIT_BREAKER_OVERHEAD_SETTING.get(settings), - ACCOUNTING_CIRCUIT_BREAKER_TYPE_SETTING.get(settings), - CircuitBreaker.Durability.PERMANENT - ) - ) - ); for (BreakerSettings breakerSettings : customBreakers) { if (childCircuitBreakers.containsKey(breakerSettings.getName())) { throw new IllegalArgumentException( @@ -290,11 +258,6 @@ public class HierarchyCircuitBreakerService extends CircuitBreakerService { REQUEST_CIRCUIT_BREAKER_OVERHEAD_SETTING, (limit, overhead) -> updateCircuitBreakerSettings(CircuitBreaker.REQUEST, limit, overhead) ); - clusterSettings.addSettingsUpdateConsumer( - ACCOUNTING_CIRCUIT_BREAKER_LIMIT_SETTING, - ACCOUNTING_CIRCUIT_BREAKER_OVERHEAD_SETTING, - (limit, overhead) -> updateCircuitBreakerSettings(CircuitBreaker.ACCOUNTING, limit, overhead) - ); clusterSettings.addAffixUpdateConsumer( CIRCUIT_BREAKER_LIMIT_SETTING, CIRCUIT_BREAKER_OVERHEAD_SETTING, diff --git a/server/src/test/java/org/opensearch/indices/breaker/HierarchyCircuitBreakerServiceTests.java b/server/src/test/java/org/opensearch/indices/breaker/HierarchyCircuitBreakerServiceTests.java index dbe05c568b3..7bdff59e7c3 100644 --- a/server/src/test/java/org/opensearch/indices/breaker/HierarchyCircuitBreakerServiceTests.java +++ b/server/src/test/java/org/opensearch/indices/breaker/HierarchyCircuitBreakerServiceTests.java @@ -275,7 +275,7 @@ public class HierarchyCircuitBreakerServiceTests extends OpenSearchTestCase { assertThat(exception.getMessage(), containsString("which is larger than the limit of [209715200/200mb]")); assertThat( exception.getMessage(), - containsString("usages [request=157286400/150mb, fielddata=54001664/51.5mb, in_flight_requests=0/0b, accounting=0/0b]") + containsString("usages [request=157286400/150mb, fielddata=54001664/51.5mb, in_flight_requests=0/0b]") ); assertThat(exception.getDurability(), equalTo(CircuitBreaker.Durability.TRANSIENT)); } @@ -341,7 +341,7 @@ public class HierarchyCircuitBreakerServiceTests extends OpenSearchTestCase { + requestCircuitBreakerUsed + "/" + new ByteSizeValue(requestCircuitBreakerUsed) - + ", fielddata=0/0b, in_flight_requests=0/0b, accounting=0/0b]" + + ", fielddata=0/0b, in_flight_requests=0/0b]" ) ); assertThat(exception.getDurability(), equalTo(CircuitBreaker.Durability.TRANSIENT)); diff --git a/test/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java b/test/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java index af59e775103..7654606767e 100644 --- a/test/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java +++ b/test/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java @@ -71,7 +71,6 @@ import org.opensearch.common.CheckedBiFunction; import org.opensearch.common.Nullable; import org.opensearch.common.Randomness; import org.opensearch.common.Strings; -import org.opensearch.common.breaker.CircuitBreaker; import org.opensearch.common.bytes.BytesArray; import org.opensearch.common.bytes.BytesReference; import org.opensearch.common.compress.CompressedXContent; @@ -345,8 +344,6 @@ public abstract class EngineTestCase extends OpenSearchTestCase { assertMaxSeqNoInCommitUserData(replicaEngine); assertAtMostOneLuceneDocumentPerSequenceNumber(replicaEngine); } - assertThat(engine.config().getCircuitBreakerService().getBreaker(CircuitBreaker.ACCOUNTING).getUsed(), equalTo(0L)); - assertThat(replicaEngine.config().getCircuitBreakerService().getBreaker(CircuitBreaker.ACCOUNTING).getUsed(), equalTo(0L)); } finally { IOUtils.close(replicaEngine, storeReplica, engine, store, () -> terminate(threadPool)); } diff --git a/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java b/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java index f078db94c64..4342f789cb0 100644 --- a/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java +++ b/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java @@ -2401,19 +2401,6 @@ public final class InternalTestCluster extends TestCluster { CircuitBreaker fdBreaker = breakerService.getBreaker(CircuitBreaker.FIELDDATA); assertThat("Fielddata breaker not reset to 0 on node: " + name, fdBreaker.getUsed(), equalTo(0L)); - try { - assertBusy(() -> { - CircuitBreaker acctBreaker = breakerService.getBreaker(CircuitBreaker.ACCOUNTING); - assertThat( - "Accounting breaker not reset to 0 on node: " + name + ", are there still Lucene indices around?", - acctBreaker.getUsed(), - equalTo(0L) - ); - }); - } catch (Exception e) { - throw new AssertionError("Exception during check for accounting breaker reset to 0", e); - } - // Anything that uses transport or HTTP can increase the // request breaker (because they use bigarrays), because of // that the breaker can sometimes be incremented from ping