SOLR-16982: Trip a Circuit Breaker only for external requests (apache…

…#1930)

Co-authored-by: Christine Poerschke <[email protected]>

solr/CHANGES.txt

@@ -113,6 +113,8 @@ Improvements
 
 * SOLR-15474: Make Circuit breakers individually pluggable (Atri Sharma, Christine Poerschke, janhoy)
 
+* SOLR-16982: Trip Circuit Breakers only for external requests (janhoy, Christine Poerschke)
+
 * SOLR-16927: Allow SolrClientCache clients to use Jetty HTTP2 clients (Alex Deparvu, David Smiley)
 
 * SOLR-16896, SOLR-16897: Add support of OAuth 2.0/OIDC 'code with PKCE' flow (Lamine Idjeraoui, janhoy, Kevin Risden, Anshum Gupta)

solr/core/src/java/org/apache/solr/handler/ContentStreamHandlerBase.java

@@ -19,6 +19,7 @@
 import static org.apache.solr.common.params.CommonParams.FAILURE;
 import static org.apache.solr.common.params.CommonParams.STATUS;
 
+import java.lang.invoke.MethodHandles;
 import java.util.List;
 import org.apache.solr.client.solrj.SolrRequest.SolrRequestType;
 import org.apache.solr.common.SolrException;
@@ -33,13 +34,17 @@
 import org.apache.solr.update.processor.UpdateRequestProcessorChain;
 import org.apache.solr.util.circuitbreaker.CircuitBreaker;
 import org.apache.solr.util.circuitbreaker.CircuitBreakerRegistry;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * Shares common code between various handlers that manipulate {@link
  * org.apache.solr.common.util.ContentStream} objects.
  */
 public abstract class ContentStreamHandlerBase extends RequestHandlerBase {
 
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
   @Override
   public void init(NamedList<?> args) {
     super.init(args);
@@ -119,6 +124,12 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
    * @return true if circuit breakers are tripped, false otherwise.
    */
   protected boolean checkCircuitBreakers(SolrQueryRequest req, SolrQueryResponse rsp) {
+    if (isInternalShardRequest(req)) {
+      if (log.isTraceEnabled()) {
+        log.trace("Internal request, skipping circuit breaker check");
+      }
+      return false;
+    }
     CircuitBreakerRegistry circuitBreakerRegistry = req.getCore().getCircuitBreakerRegistry();
     if (circuitBreakerRegistry.isEnabled(SolrRequestType.UPDATE)) {
       List<CircuitBreaker> trippedCircuitBreakers =

solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java

@@ -28,6 +28,7 @@
 import org.apache.solr.api.ApiBag;
 import org.apache.solr.api.ApiSupport;
 import org.apache.solr.common.SolrException;
+import org.apache.solr.common.params.ShardParams;
 import org.apache.solr.common.params.SolrParams;
 import org.apache.solr.common.util.NamedList;
 import org.apache.solr.common.util.SuppressForbidden;
@@ -44,6 +45,7 @@
 import org.apache.solr.response.SolrQueryResponse;
 import org.apache.solr.search.SyntaxError;
 import org.apache.solr.security.PermissionNameProvider;
+import org.apache.solr.update.processor.DistributedUpdateProcessor;
 import org.apache.solr.util.SolrPluginUtils;
 import org.apache.solr.util.TestInjection;
 import org.slf4j.Logger;
@@ -343,4 +345,16 @@ public Collection<Api> getApis() {
     return Collections.singleton(
         new ApiBag.ReqHandlerToApi(this, ApiBag.constructSpec(pluginInfo)));
   }
+
+  /**
+   * Checks whether the given request is an internal request to a shard. We rely on the fact that an
+   * internal search request to a shard contains the param "isShard", and an internal update request
+   * to a shard contains the param "distrib.from".
+   *
+   * @return true if request is internal
+   */
+  public static boolean isInternalShardRequest(SolrQueryRequest req) {
+    return req.getParams().get(DistributedUpdateProcessor.DISTRIB_FROM) != null
+        || "true".equals(req.getParams().get(ShardParams.IS_SHARD));
+  }
 }

solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java

@@ -357,6 +357,12 @@ protected ResponseBuilder newResponseBuilder(
    */
   protected boolean checkCircuitBreakers(
       SolrQueryRequest req, SolrQueryResponse rsp, ResponseBuilder rb) {
+    if (isInternalShardRequest(req)) {
+      if (log.isTraceEnabled()) {
+        log.trace("Internal request, skipping circuit breaker check");
+      }
+      return false;
+    }
     final RTimerTree timer = rb.isDebug() ? req.getRequestTimer() : null;
 
     final CircuitBreakerRegistry circuitBreakerRegistry = req.getCore().getCircuitBreakerRegistry();

solr/core/src/test/org/apache/solr/handler/RequestHandlerBaseTest.java

@@ -26,8 +26,10 @@
 import com.codahale.metrics.Counter;
 import com.codahale.metrics.Meter;
 import com.codahale.metrics.Timer;
+import java.util.Map;
 import org.apache.solr.SolrTestCaseJ4;
 import org.apache.solr.common.SolrException;
+import org.apache.solr.common.params.MapSolrParams;
 import org.apache.solr.common.params.ModifiableSolrParams;
 import org.apache.solr.core.CoreContainer;
 import org.apache.solr.core.SolrCore;
@@ -145,6 +147,25 @@ public CoreContainer getCoreContainer() {
     assertEquals(SolrException.ErrorCode.SERVER_ERROR.code, normalizedSolrException.code());
   }
 
+  @Test
+  public void testIsInternalShardRequest() {
+    final SolrQueryRequest solrQueryRequest =
+        new LocalSolrQueryRequest(solrCore, new ModifiableSolrParams()) {
+          @Override
+          public CoreContainer getCoreContainer() {
+            return coreContainer;
+          }
+        };
+
+    assertFalse(RequestHandlerBase.isInternalShardRequest(solrQueryRequest));
+
+    solrQueryRequest.setParams(new MapSolrParams(Map.of("isShard", "true")));
+    assertTrue(RequestHandlerBase.isInternalShardRequest(solrQueryRequest));
+
+    solrQueryRequest.setParams(new MapSolrParams(Map.of("distrib.from", "http://foo:1234/solr")));
+    assertTrue(RequestHandlerBase.isInternalShardRequest(solrQueryRequest));
+  }
+
   // Ideally we wouldn't need to use mocks here, but HandlerMetrics requires a SolrMetricsContext,
   // which
   //  requires a MetricsManager, which requires ...