From 74ac97e40252a8808c882a22b25181fa999af276 Mon Sep 17 00:00:00 2001 From: Atri Sharma Date: Fri, 26 Jun 2020 22:20:21 +0530 Subject: [PATCH] SOLR-14588: Follow Up Fixes and Documentation (#1615) This commit is a follow up to the original commit and adds more documentation and adds timing information for circuit breaker in query response only if circuit breakers are enabled. This commit also adds a test for ensuring that the query response is correct when timing is enabled and circuit breakers are being used. --- solr/CHANGES.txt | 2 +- .../solr/handler/component/SearchHandler.java | 36 +++++++++--------- .../util/circuitbreaker/CircuitBreaker.java | 5 +++ .../circuitbreaker/MemoryCircuitBreaker.java | 9 +++++ .../handler/component/DebugComponentTest.java | 12 ++---- .../apache/solr/util/TestCircuitBreaker.java | 37 +++++++++++++++++++ solr/example/files/conf/solrconfig.xml | 22 ++++++++++- 7 files changed, 94 insertions(+), 29 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 2a7d6dd27fd..57bab2152fb 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -12,7 +12,7 @@ New Features --------------------- * SOLR-14440: Introduce new Certificate Authentication Plugin to load Principal from certificate subject. (Mike Drob) -* SOLR-14588: Implement Circuit Breakers Infrastructure and add JVM Circuit Breaker (Atri Sharma) +* SOLR-14588: Implement Circuit Breakers Infrastructure and add max JVM usage based circuit breaker (Atri Sharma) Improvements ---------------------- 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 9b49f3f84e5..b85ed305daf 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 @@ -303,27 +303,27 @@ public class SearchHandler extends RequestHandlerBase implements SolrCoreAware, final RTimerTree timer = rb.isDebug() ? req.getRequestTimer() : null; - Map trippedCircuitBreakers; + if (req.getCore().getSolrConfig().useCircuitBreakers) { + Map trippedCircuitBreakers; + if (timer != null) { + RTimerTree subt = timer.sub("circuitbreaker"); + rb.setTimer(subt); - if (timer != null) { - RTimerTree subt = timer.sub("circuitbreaker"); - rb.setTimer(subt.sub("circuitbreaker")); + CircuitBreakerManager circuitBreakerManager = req.getCore().getCircuitBreakerManager(); + trippedCircuitBreakers = circuitBreakerManager.checkAllCircuitBreakers(); - CircuitBreakerManager circuitBreakerManager = req.getCore().getCircuitBreakerManager(); - trippedCircuitBreakers = circuitBreakerManager.checkAllCircuitBreakers(); + rb.getTimer().stop(); + } else { + CircuitBreakerManager circuitBreakerManager = req.getCore().getCircuitBreakerManager(); + trippedCircuitBreakers = circuitBreakerManager.checkAllCircuitBreakers(); + } - rb.getTimer().stop(); - subt.stop(); - } else { - CircuitBreakerManager circuitBreakerManager = req.getCore().getCircuitBreakerManager(); - trippedCircuitBreakers = circuitBreakerManager.checkAllCircuitBreakers(); - } - - if (trippedCircuitBreakers != null) { - String errorMessage = CircuitBreakerManager.constructFinalErrorMessageString(trippedCircuitBreakers); - rsp.add(STATUS, FAILURE); - rsp.setException(new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Circuit Breakers tripped " + errorMessage)); - return; + if (trippedCircuitBreakers != null) { + String errorMessage = CircuitBreakerManager.constructFinalErrorMessageString(trippedCircuitBreakers); + rsp.add(STATUS, FAILURE); + rsp.setException(new SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Circuit Breakers tripped " + errorMessage)); + return; + } } final ShardHandler shardHandler1 = getAndPrepShardHandler(req, rb); // creates a ShardHandler object only if it's needed diff --git a/solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreaker.java b/solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreaker.java index 2e7de82857b..290b5616f59 100644 --- a/solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreaker.java +++ b/solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreaker.java @@ -22,6 +22,11 @@ import org.apache.solr.core.SolrCore; /** * Default class to define circuit breakers for Solr. * + * There are two (typical) ways to use circuit breakers: + * + * 1. Have them checked at admission control by default (use CircuitBreakerManager for the same) + * 2. Use the circuit breaker in a specific code path(s) + * * TODO: This class should be grown as the scope of circuit breakers grow. */ public abstract class CircuitBreaker { diff --git a/solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java b/solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java index ae067e97e45..384b95f9971 100644 --- a/solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java +++ b/solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java @@ -22,6 +22,15 @@ import java.lang.management.MemoryMXBean; import org.apache.solr.core.SolrCore; +/** + * Tracks the current JVM heap usage and triggers if it exceeds the defined percentage of the maximum + * heap size allocated to the JVM. This circuit breaker is a part of the default CircuitBreakerManager + * so is checked for every request -- hence it is realtime. Once the memory usage goes below the threshold, + * it will start allowing queries again. + * + * The memory threshold is defined as a percentage of the maximum memory allocated -- see memoryCircuitBreakerThreshold + * in solrconfig.xml + */ public class MemoryCircuitBreaker extends CircuitBreaker { private static final MemoryMXBean MEMORY_MX_BEAN = ManagementFactory.getMemoryMXBean(); diff --git a/solr/core/src/test/org/apache/solr/handler/component/DebugComponentTest.java b/solr/core/src/test/org/apache/solr/handler/component/DebugComponentTest.java index 5134d38bb19..92430886db9 100644 --- a/solr/core/src/test/org/apache/solr/handler/component/DebugComponentTest.java +++ b/solr/core/src/test/org/apache/solr/handler/component/DebugComponentTest.java @@ -59,10 +59,8 @@ public class DebugComponentTest extends SolrTestCaseJ4 { "//lst[@name='explain']/str[@name='2']", "//lst[@name='explain']/str[@name='3']", "//str[@name='QParser']",// make sure the QParser is specified - "count(//lst[@name='timing']/*)=4", //should be four pieces to timings + "count(//lst[@name='timing']/*)=3", //should be three pieces to timings "//lst[@name='timing']/double[@name='time']", //make sure we have a time value, but don't specify its result - "count(//lst[@name='circuitbreaker']/*)>0", - "//lst[@name='circuitbreaker']/double[@name='time']", "count(//lst[@name='prepare']/*)>0", "//lst[@name='prepare']/double[@name='time']", "count(//lst[@name='process']/*)>0", @@ -85,10 +83,8 @@ public class DebugComponentTest extends SolrTestCaseJ4 { "//lst[@name='explain']/str[@name='1']", "//lst[@name='explain']/str[@name='2']", "//lst[@name='explain']/str[@name='3']", - "count(//lst[@name='timing']/*)=4", //should be four pieces to timings + "count(//lst[@name='timing']/*)=3", //should be four pieces to timings "//lst[@name='timing']/double[@name='time']", //make sure we have a time value, but don't specify its result - "count(//lst[@name='circuitbreaker']/*)>0", - "//lst[@name='circuitbreaker']/double[@name='time']", "count(//lst[@name='prepare']/*)>0", "//lst[@name='prepare']/double[@name='time']", "count(//lst[@name='process']/*)>0", @@ -102,10 +98,8 @@ public class DebugComponentTest extends SolrTestCaseJ4 { "count(//str[@name='parsedquery_toString'])=0", "count(//lst[@name='explain']/*)=0", "count(//str[@name='QParser'])=0",// make sure the QParser is specified - "count(//lst[@name='timing']/*)=4", //should be four pieces to timings + "count(//lst[@name='timing']/*)=3", //should be four pieces to timings "//lst[@name='timing']/double[@name='time']", //make sure we have a time value, but don't specify its result - "count(//lst[@name='circuitbreaker']/*)>0", - "//lst[@name='circuitbreaker']/double[@name='time']", "count(//lst[@name='prepare']/*)>0", "//lst[@name='prepare']/double[@name='time']", "count(//lst[@name='process']/*)>0", diff --git a/solr/core/src/test/org/apache/solr/util/TestCircuitBreaker.java b/solr/core/src/test/org/apache/solr/util/TestCircuitBreaker.java index ad6295dd43a..7807b30fce1 100644 --- a/solr/core/src/test/org/apache/solr/util/TestCircuitBreaker.java +++ b/solr/core/src/test/org/apache/solr/util/TestCircuitBreaker.java @@ -50,6 +50,9 @@ public class TestCircuitBreaker extends SolrTestCaseJ4 { assertU(adoc("name", "john smith", "id", "1")); assertU(adoc("name", "johathon smith", "id", "2")); assertU(adoc("name", "john percival smith", "id", "3")); + assertU(adoc("id", "1", "title", "this is a title.", "inStock_b1", "true")); + assertU(adoc("id", "2", "title", "this is another title.", "inStock_b1", "true")); + assertU(adoc("id", "3", "title", "Mary had a little lamb.", "inStock_b1", "false")); //commit inside the loop to get multiple segments to make search as realistic as possible assertU(commit()); @@ -68,6 +71,28 @@ public class TestCircuitBreaker extends SolrTestCaseJ4 { System.clearProperty("documentCache.enabled"); } + public void testResponseWithCBTiming() { + assertQ(req("q", "*:*", CommonParams.DEBUG_QUERY, "true"), + "//str[@name='rawquerystring']='*:*'", + "//str[@name='querystring']='*:*'", + "//str[@name='parsedquery']='MatchAllDocsQuery(*:*)'", + "//str[@name='parsedquery_toString']='*:*'", + "count(//lst[@name='explain']/*)=3", + "//lst[@name='explain']/str[@name='1']", + "//lst[@name='explain']/str[@name='2']", + "//lst[@name='explain']/str[@name='3']", + "//str[@name='QParser']", + "count(//lst[@name='timing']/*)=4", + "//lst[@name='timing']/double[@name='time']", + "count(//lst[@name='circuitbreaker']/*)>0", + "//lst[@name='circuitbreaker']/double[@name='time']", + "count(//lst[@name='prepare']/*)>0", + "//lst[@name='prepare']/double[@name='time']", + "count(//lst[@name='process']/*)>0", + "//lst[@name='process']/double[@name='time']" + ); + } + public void testCBAlwaysTrips() throws IOException { HashMap args = new HashMap(); @@ -81,6 +106,10 @@ public class TestCircuitBreaker extends SolrTestCaseJ4 { expectThrows(SolrException.class, () -> { h.query(req("name:\"john smith\"")); }); + + circuitBreaker = new MemoryCircuitBreaker(h.getCore()); + + h.getCore().getCircuitBreakerManager().registerCircuitBreaker(CircuitBreakerType.MEMORY, circuitBreaker); } public void testCBFakeMemoryPressure() throws IOException { @@ -96,6 +125,10 @@ public class TestCircuitBreaker extends SolrTestCaseJ4 { expectThrows(SolrException.class, () -> { h.query(req("name:\"john smith\"")); }); + + circuitBreaker = new MemoryCircuitBreaker(h.getCore()); + + h.getCore().getCircuitBreakerManager().registerCircuitBreaker(CircuitBreakerType.MEMORY, circuitBreaker); } public void testBuildingMemoryPressure() { @@ -133,6 +166,10 @@ public class TestCircuitBreaker extends SolrTestCaseJ4 { } assertEquals("Number of failed queries is not correct", 1, failureCount.get()); + + circuitBreaker = new MemoryCircuitBreaker(h.getCore()); + + h.getCore().getCircuitBreakerManager().registerCircuitBreaker(CircuitBreakerType.MEMORY, circuitBreaker); } finally { if (!executor.isShutdown()) { executor.shutdown(); diff --git a/solr/example/files/conf/solrconfig.xml b/solr/example/files/conf/solrconfig.xml index c400a44cf93..b85657034f4 100644 --- a/solr/example/files/conf/solrconfig.xml +++ b/solr/example/files/conf/solrconfig.xml @@ -499,12 +499,32 @@ 200 false 100