You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@solr.apache.org by ja...@apache.org on 2023/09/22 07:36:04 UTC
[solr] branch main updated: SOLR-16982: Trip a Circuit Breaker only for external requests (#1930)
This is an automated email from the ASF dual-hosted git repository.
janhoy pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/solr.git
The following commit(s) were added to refs/heads/main by this push:
new d9c65fb1a64 SOLR-16982: Trip a Circuit Breaker only for external requests (#1930)
d9c65fb1a64 is described below
commit d9c65fb1a64f6e85d5b869519ff2dbc1f529cb65
Author: Jan Høydahl <ja...@users.noreply.github.com>
AuthorDate: Fri Sep 22 09:35:57 2023 +0200
SOLR-16982: Trip a Circuit Breaker only for external requests (#1930)
Co-authored-by: Christine Poerschke <cp...@apache.org>
---
solr/CHANGES.txt | 2 ++
.../solr/handler/ContentStreamHandlerBase.java | 11 +++++++++++
.../org/apache/solr/handler/RequestHandlerBase.java | 14 ++++++++++++++
.../solr/handler/component/SearchHandler.java | 6 ++++++
.../apache/solr/handler/RequestHandlerBaseTest.java | 21 +++++++++++++++++++++
5 files changed, 54 insertions(+)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 24ec5b37e1e..70a93e99ab6 100644
--- a/solr/CHANGES.txt
+++ b/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)
diff --git a/solr/core/src/java/org/apache/solr/handler/ContentStreamHandlerBase.java b/solr/core/src/java/org/apache/solr/handler/ContentStreamHandlerBase.java
index fb40af3c081..f28a7a6a6de 100644
--- a/solr/core/src/java/org/apache/solr/handler/ContentStreamHandlerBase.java
+++ b/solr/core/src/java/org/apache/solr/handler/ContentStreamHandlerBase.java
@@ -19,6 +19,7 @@ package org.apache.solr.handler;
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,6 +34,8 @@ import org.apache.solr.update.processor.UpdateRequestProcessor;
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
@@ -40,6 +43,8 @@ import org.apache.solr.util.circuitbreaker.CircuitBreakerRegistry;
*/
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 abstract class ContentStreamHandlerBase extends RequestHandlerBase {
* @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 =
diff --git a/solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java b/solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java
index 3a3c89f2b8a..2c53eb26625 100644
--- a/solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java
+++ b/solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java
@@ -28,6 +28,7 @@ import org.apache.solr.api.Api;
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.request.SolrRequestHandler;
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 abstract class RequestHandlerBase
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));
+ }
}
diff --git a/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java b/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java
index 995e6a3c684..744d6093222 100644
--- a/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java
+++ b/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java
@@ -357,6 +357,12 @@ public class SearchHandler extends RequestHandlerBase
*/
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();
diff --git a/solr/core/src/test/org/apache/solr/handler/RequestHandlerBaseTest.java b/solr/core/src/test/org/apache/solr/handler/RequestHandlerBaseTest.java
index 455f1c26df9..c3f2f9bd865 100644
--- a/solr/core/src/test/org/apache/solr/handler/RequestHandlerBaseTest.java
+++ b/solr/core/src/test/org/apache/solr/handler/RequestHandlerBaseTest.java
@@ -26,8 +26,10 @@ import static org.mockito.Mockito.when;
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 class RequestHandlerBaseTest extends SolrTestCaseJ4 {
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 ...