madrob commented on a change in pull request #1737:
URL: https://github.com/apache/lucene-solr/pull/1737#discussion_r473196288
##########
File path:
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java
##########
@@ -88,13 +95,20 @@ public boolean isTripped() {
@Override
public String getDebugInfo() {
- if (seenMemory.get() == 0L || allowedMemory.get() == 0L) {
+ if (seenMemory.get() == 0.0 || allowedMemory.get() == 0.0) {
Review comment:
I don't think this was supposed to change
##########
File path: solr/core/src/test/org/apache/solr/util/TestCircuitBreaker.java
##########
@@ -119,33 +130,42 @@ public void testBuildingMemoryPressure() {
AtomicInteger failureCount = new AtomicInteger();
try {
+ removeAllExistingCircuitBreakers();
+
CircuitBreaker circuitBreaker = new
BuildingUpMemoryPressureCircuitBreaker(h.getCore().getSolrConfig());
h.getCore().getCircuitBreakerManager().register(circuitBreaker);
+ List<Future> futures = new ArrayList<>();
+
for (int i = 0; i < 5; i++) {
- executor.submit(() -> {
+ Future future = executor.submit(() -> {
Review comment:
`Future<?>`
##########
File path: solr/solr-ref-guide/src/circuit-breakers.adoc
##########
@@ -32,36 +32,58 @@ will be disabled globally. Per circuit breaker
configurations are specified in t
[source,xml]
----
-<useCircuitBreakers>false</useCircuitBreakers>
+<circuitBreakers enabled="true">
+ <!-- All specific configs in this section -->
+</circuitBreakers>
----
+This flag acts as the highest authority and global controller of circuit
breakers. For using specific circuit breakers, each one
+needs to be individually enabled in addition to this flag being enabled.
+
== Currently Supported Circuit Breakers
=== JVM Heap Usage Based Circuit Breaker
This circuit breaker tracks JVM heap memory usage and rejects incoming search
requests with a 503 error code if the heap usage
exceeds a configured percentage of maximum heap allocated to the JVM (-Xmx).
The main configuration for this circuit breaker is
controlling the threshold percentage at which the breaker will trip.
-It does not logically make sense to have a threshold below 50% and above 95%
of the max heap allocated to the JVM. Hence, the range
-of valid values for this parameter is [50, 95], both inclusive.
+Configuration for JVM heap usage based circuit breaker:
[source,xml]
----
-<memoryCircuitBreakerThresholdPct>75</memoryCircuitBreakerThresholdPct>
+<memBreaker enabled="true" threshold="75"/>
----
+Note that this configuration will be overridden by the global circuit breaker
flag -- if circuit breakers are disabled, this flag
+will not help you. Also, the triggering threshold is defined as a percentage
of the max heap allocated to the JVM.
+
+It does not logically make sense to have a threshold below 50% and above 95%
of the max heap allocated to the JVM. Hence, the range
+of valid values for this parameter is [50, 95], both inclusive.
+
Consider the following example:
JVM has been allocated a maximum heap of 5GB (-Xmx) and
memoryCircuitBreakerThresholdPct is set to 75. In this scenario, the heap usage
at which the circuit breaker will trip is 3.75GB.
-Note that this circuit breaker is checked for each incoming search request and
considers the current heap usage of the node, i.e every search
-request will get the live heap usage and compare it against the set memory
threshold. The check does not impact performance,
-but any performance regressions that are suspected to be caused by this
feature should be reported to the dev list.
+=== CPU Utilization Based Circuit Breaker
Review comment:
@sigram we could call this Load Avg Based Circuit Breaker, WDYT? Maybe
that is too much implementation detail?
##########
File path: solr/server/solr/configsets/_default/conf/solrconfig.xml
##########
@@ -615,10 +612,29 @@
was caused by tripped circuit breakers).
-->
<!--
- <memoryCircuitBreakerThresholdPct>100</memoryCircuitBreakerThresholdPct>
+ <memBreaker enabled="true" threshold="75"/>
-->
- </circuitBreaker>
+ <!-- CPU Circuit Breaker Configuration
+
+ Specific configuration for CPU utilization based circuit breaker. This
configuration defines whether the circuit breaker is enabled
+ and the average load over the last minute at which the circuit breaker
should start rejecting queries.
+
+ Consider a scenario where the max heap allocated is 4 GB and
memoryCircuitBreakerThreshold is
+ defined as 75. Threshold JVM usage will be 4 * 0.75 = 3 GB. Its generally
a good idea to keep this value between 75 - 80% of maximum heap
+ allocated.
+
+ If, at any point, the current JVM heap usage goes above 3 GB, queries
will be rejected until the heap usage goes below 3 GB again.
+ If you see queries getting rejected with 503 error code, check for
"Circuit Breakers tripped"
+ in logs and the corresponding error message should tell you what
transpired (if the failure
+ was caused by tripped circuit breakers).
Review comment:
This is not part of this CB, right?
##########
File path: solr/core/src/test/org/apache/solr/util/TestCircuitBreaker.java
##########
@@ -41,6 +45,9 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import static org.hamcrest.CoreMatchers.containsString;
+
+@SuppressWarnings({"rawtypes"})
Review comment:
Do we have to suppress here? I'd rather not have warnings in the new
code to begin with
##########
File path: solr/core/src/test/org/apache/solr/util/TestCircuitBreaker.java
##########
@@ -157,6 +177,60 @@ public void testBuildingMemoryPressure() {
}
}
+ public void testFakeCPUCircuitBreaker() {
+ ExecutorService executor = ExecutorUtil.newMDCAwareCachedThreadPool(
Review comment:
Better practice to declare this right before the `try` block, without
intervening declarations.
##########
File path:
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java
##########
@@ -43,22 +43,25 @@
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
private static final MemoryMXBean MEMORY_MX_BEAN =
ManagementFactory.getMemoryMXBean();
+ private boolean enabled;
private final long heapMemoryThreshold;
// Assumption -- the value of these parameters will be set correctly before
invoking getDebugInfo()
- private final ThreadLocal<Long> seenMemory = new ThreadLocal<>();
- private final ThreadLocal<Long> allowedMemory = new ThreadLocal<>();
+ private final ThreadLocal<Long> seenMemory = ThreadLocal.withInitial(() ->
0L);
Review comment:
static
##########
File path: solr/core/src/test/org/apache/solr/util/TestCircuitBreaker.java
##########
@@ -206,11 +286,12 @@ public FakeMemoryPressureCircuitBreaker(SolrConfig
solrConfig) {
@Override
protected long calculateLiveMemoryUsage() {
// Return a number large enough to trigger a pushback from the circuit
breaker
+ System.out.println("I AM SENDING MAX VALUE");
Review comment:
log, not sys out.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
