From c13df3b6c5020c185c232c590132151896d48d03 Mon Sep 17 00:00:00 2001
From: Daniel Mitterdorfer <daniel.mitterdorfer@elastic.co>
Date: Wed, 18 May 2016 09:18:10 +0200
Subject: [PATCH] Clear all caches after testing parent breaker

With this commit we clear all caches after testing the parent circuit breaker.
This is necessary as caches hold on to circuit breakers internally. Additionally,
due to usage of CircuitBreaker#addWithoutBreaking() in caches, it's even possible
to go above the limit. As a consequence, all subsequent requests fall victim to
the limit.

Hence, right after the parent circuit breaker tripped, we clear all caches to
reduce these circuit breakers to 0 again. We also exclude the clear caches
transport request from limit check in order to ensure it will succeed. As this is
typically a very small and low-volume request, it is deemed ok to exclude it.

Closes #18325
---
 .../TransportClearIndicesCacheAction.java     |  2 +-
 .../node/TransportBroadcastByNodeAction.java  | 20 +++++++++++++++++--
 .../breaker/CircuitBreakerServiceIT.java      |  7 +++----
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/core/src/main/java/org/elasticsearch/action/admin/indices/cache/clear/TransportClearIndicesCacheAction.java b/core/src/main/java/org/elasticsearch/action/admin/indices/cache/clear/TransportClearIndicesCacheAction.java
index 59cd95044cc..fe97302060d 100644
--- a/core/src/main/java/org/elasticsearch/action/admin/indices/cache/clear/TransportClearIndicesCacheAction.java
+++ b/core/src/main/java/org/elasticsearch/action/admin/indices/cache/clear/TransportClearIndicesCacheAction.java
@@ -54,7 +54,7 @@ public class TransportClearIndicesCacheAction extends TransportBroadcastByNodeAc
                                             TransportService transportService, IndicesService indicesService, ActionFilters actionFilters,
                                             IndexNameExpressionResolver indexNameExpressionResolver) {
         super(settings, ClearIndicesCacheAction.NAME, threadPool, clusterService, transportService, actionFilters, indexNameExpressionResolver,
-                ClearIndicesCacheRequest::new, ThreadPool.Names.MANAGEMENT);
+                ClearIndicesCacheRequest::new, ThreadPool.Names.MANAGEMENT, false);
         this.indicesService = indicesService;
     }
 
diff --git a/core/src/main/java/org/elasticsearch/action/support/broadcast/node/TransportBroadcastByNodeAction.java b/core/src/main/java/org/elasticsearch/action/support/broadcast/node/TransportBroadcastByNodeAction.java
index 29863419a4e..3356d189143 100644
--- a/core/src/main/java/org/elasticsearch/action/support/broadcast/node/TransportBroadcastByNodeAction.java
+++ b/core/src/main/java/org/elasticsearch/action/support/broadcast/node/TransportBroadcastByNodeAction.java
@@ -84,6 +84,20 @@ public abstract class TransportBroadcastByNodeAction<Request extends BroadcastRe
 
     final String transportNodeBroadcastAction;
 
+    public TransportBroadcastByNodeAction(
+        Settings settings,
+        String actionName,
+        ThreadPool threadPool,
+        ClusterService clusterService,
+        TransportService transportService,
+        ActionFilters actionFilters,
+        IndexNameExpressionResolver indexNameExpressionResolver,
+        Supplier<Request> request,
+        String executor) {
+        this(settings, actionName, threadPool, clusterService, transportService, actionFilters, indexNameExpressionResolver, request,
+            executor, true);
+    }
+
     public TransportBroadcastByNodeAction(
             Settings settings,
             String actionName,
@@ -93,7 +107,8 @@ public abstract class TransportBroadcastByNodeAction<Request extends BroadcastRe
             ActionFilters actionFilters,
             IndexNameExpressionResolver indexNameExpressionResolver,
             Supplier<Request> request,
-            String executor) {
+            String executor,
+            boolean canTripCircuitBreaker) {
         super(settings, actionName, threadPool, transportService, actionFilters, indexNameExpressionResolver, request);
 
         this.clusterService = clusterService;
@@ -101,7 +116,8 @@ public abstract class TransportBroadcastByNodeAction<Request extends BroadcastRe
 
         transportNodeBroadcastAction = actionName + "[n]";
 
-        transportService.registerRequestHandler(transportNodeBroadcastAction, NodeRequest::new, executor, new BroadcastByNodeTransportRequestHandler());
+        transportService.registerRequestHandler(transportNodeBroadcastAction, NodeRequest::new, executor, false, canTripCircuitBreaker,
+            new BroadcastByNodeTransportRequestHandler());
     }
 
     private Response newResponse(
diff --git a/core/src/test/java/org/elasticsearch/indices/memory/breaker/CircuitBreakerServiceIT.java b/core/src/test/java/org/elasticsearch/indices/memory/breaker/CircuitBreakerServiceIT.java
index a125e3842e7..0ae8f71c742 100644
--- a/core/src/test/java/org/elasticsearch/indices/memory/breaker/CircuitBreakerServiceIT.java
+++ b/core/src/test/java/org/elasticsearch/indices/memory/breaker/CircuitBreakerServiceIT.java
@@ -75,6 +75,9 @@ public class CircuitBreakerServiceIT extends ESIntegTestCase {
     /** Reset all breaker settings back to their defaults */
     private void reset() {
         logger.info("--> resetting breaker settings");
+        // clear all caches, we could be very close (or even above) the limit and then we will not be able to reset the breaker settings
+        client().admin().indices().prepareClearCache().setFieldDataCache(true).setQueryCache(true).setRequestCache(true).get();
+
         Settings resetSettings = Settings.builder()
                 .put(HierarchyCircuitBreakerService.FIELDDATA_CIRCUIT_BREAKER_LIMIT_SETTING.getKey(),
                         HierarchyCircuitBreakerService.FIELDDATA_CIRCUIT_BREAKER_LIMIT_SETTING.getDefaultRaw(null))
@@ -214,7 +217,6 @@ public class CircuitBreakerServiceIT extends ESIntegTestCase {
      * Test that a breaker correctly redistributes to a different breaker, in
      * this case, the fielddata breaker borrows space from the request breaker
      */
-    @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/18325")
     public void testParentChecking() throws Exception {
         if (noopBreakerUsed()) {
             logger.info("--> noop breakers used, skipping test");
@@ -274,9 +276,6 @@ public class CircuitBreakerServiceIT extends ESIntegTestCase {
                 cause.toString(), startsWith("CircuitBreakingException[[parent] Data too large"));
             assertThat("Exception: [" + cause.toString() + "] should contain a CircuitBreakingException",
                 cause.toString(), endsWith(errMsg));
-        } finally {
-            // reset before teardown as it requires properly set up breakers
-            reset();
         }
     }