madrob commented on a change in pull request #1737:
URL: https://github.com/apache/lucene-solr/pull/1737#discussion_r468773336



##########
File path: solr/core/src/test/org/apache/solr/util/TestCircuitBreaker.java
##########
@@ -240,4 +303,15 @@ protected long calculateLiveMemoryUsage() {
       return Long.MIN_VALUE; // Random number guaranteed to not trip the 
circuit breaker
     }
   }
+
+  private class FakeCPUCircuitBreaker extends CPUCircuitBreaker {

Review comment:
       class can be static

##########
File path: solr/core/src/test/org/apache/solr/util/TestCircuitBreaker.java
##########
@@ -157,6 +163,57 @@ public void testBuildingMemoryPressure() {
     }
   }
 
+  public void testFakeCPUCircuitBreaker() {
+    ExecutorService executor = ExecutorUtil.newMDCAwareCachedThreadPool(
+        new SolrNamedThreadFactory("TestCircuitBreaker"));
+    HashMap<String, String> args = new HashMap<String, String>();

Review comment:
       these are unused

##########
File path: solr/core/src/test/org/apache/solr/util/TestCircuitBreaker.java
##########
@@ -157,6 +163,57 @@ public void testBuildingMemoryPressure() {
     }
   }
 
+  public void testFakeCPUCircuitBreaker() {
+    ExecutorService executor = ExecutorUtil.newMDCAwareCachedThreadPool(
+        new SolrNamedThreadFactory("TestCircuitBreaker"));
+    HashMap<String, String> args = new HashMap<String, String>();
+
+    args.put(QueryParsing.DEFTYPE, CircuitBreaker.NAME);
+    args.put(CommonParams.FL, "id");
+
+    AtomicInteger failureCount = new AtomicInteger();
+
+    try {
+      removeAllExistingCircuitBreakers();
+
+      CircuitBreaker circuitBreaker = new 
FakeCPUCircuitBreaker(h.getCore().getSolrConfig());
+
+      h.getCore().getCircuitBreakerManager().register(circuitBreaker);
+
+      for (int i = 0; i < 5; i++) {
+        executor.submit(() -> {
+          try {
+            h.query(req("name:\"john smith\""));
+          } catch (SolrException e) {
+            if (!e.getMessage().startsWith("Circuit Breakers tripped")) {
+              if (log.isInfoEnabled()) {
+                String logMessage = "Expected error message for 
testFakeCPUCircuitBreaker was not received. Error message " + e.getMessage();
+                log.info(logMessage);
+              }
+              throw new RuntimeException("Expected error message was not 
received. Error message " + e.getMessage());
+            }

Review comment:
       Can we simplify this whole block to `assertThat(e.getMessage(), 
containsString("Circuit Breakers tripped"))`

##########
File path: solr/core/src/test/org/apache/solr/util/TestCircuitBreaker.java
##########
@@ -157,6 +163,57 @@ public void testBuildingMemoryPressure() {
     }
   }
 
+  public void testFakeCPUCircuitBreaker() {
+    ExecutorService executor = ExecutorUtil.newMDCAwareCachedThreadPool(
+        new SolrNamedThreadFactory("TestCircuitBreaker"));
+    HashMap<String, String> args = new HashMap<String, String>();
+
+    args.put(QueryParsing.DEFTYPE, CircuitBreaker.NAME);
+    args.put(CommonParams.FL, "id");
+
+    AtomicInteger failureCount = new AtomicInteger();
+
+    try {
+      removeAllExistingCircuitBreakers();
+
+      CircuitBreaker circuitBreaker = new 
FakeCPUCircuitBreaker(h.getCore().getSolrConfig());
+
+      h.getCore().getCircuitBreakerManager().register(circuitBreaker);
+
+      for (int i = 0; i < 5; i++) {
+        executor.submit(() -> {

Review comment:
       save this `Future` and assert that it did not throw an exception

##########
File path: solr/solr-ref-guide/src/circuit-breakers.adoc
##########
@@ -35,33 +35,67 @@ will be disabled globally. Per circuit breaker 
configurations are specified in t
 <useCircuitBreakers>false</useCircuitBreakers>
 ----
 
+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.
+To enable/disable JVM heap usage based circuit breaker, use the following 
configuration:
+
+[source,xml]
+----
+<isMemoryCircuitBreakerEnabled>true</isMemoryCircuitBreakerEnabled>
+----
+
+Note that this configuration will be overridden by the global circuit breaker 
flag -- if circuit breakers are disabled, this flag
+will not help you.
+
+To set the triggering threshold as a percentage of the max heap allocated to 
the JVM, use the following parameter.
 
 [source,xml]
 ----
 <memoryCircuitBreakerThresholdPct>75</memoryCircuitBreakerThresholdPct>
 ----
+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
+This circuit breaker tracks CPU utilization and triggers if the average CPU 
utilization over the last minute
+exceeds a configurable threshold.
+
+To enable/disable CPU utilization based circuit breaker, use the following 
configuration:
+
+[source,xml]
+----
+<isCpuCircuitBreakerEnabled>true</isCpuCircuitBreakerEnabled>
+----
+
+Note that this configuration will be overridden by the global circuit breaker 
flag -- if circuit breakers are disabled, this flag
+will not help you.
+
+To set the triggering threshold in units of CPU utilization, use the following 
parameter.
+[source,xml]
+----
+<cpuCircuitBreakerThreshold>75</cpuCircuitBreakerThreshold>
+----
+The range of valid values for this parameter is [40 to infinity].
+
+Note that both these circuit breakers are checked for each incoming search 
request and consider the current heap usage of the node and average CPU usage 
over the last minute, respectively.

Review comment:
       Be more explicit about the CPU check using the "1 min load avg"
   
   Since this is a relatively complicated metric (compared to straight memory 
usage) let's also provide a link - "For more information on how load avg is 
calculated refer to https://en.wikipedia.org/wiki/Load_(computing)"
   
   Is this the same on Windows? Can we enlist one of our Windows install havers 
to check this out? @dweiss ?

##########
File path: solr/solr-ref-guide/src/circuit-breakers.adoc
##########
@@ -35,33 +35,67 @@ will be disabled globally. Per circuit breaker 
configurations are specified in t
 <useCircuitBreakers>false</useCircuitBreakers>
 ----
 
+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.
+To enable/disable JVM heap usage based circuit breaker, use the following 
configuration:
+
+[source,xml]
+----
+<isMemoryCircuitBreakerEnabled>true</isMemoryCircuitBreakerEnabled>
+----
+
+Note that this configuration will be overridden by the global circuit breaker 
flag -- if circuit breakers are disabled, this flag
+will not help you.
+
+To set the triggering threshold as a percentage of the max heap allocated to 
the JVM, use the following parameter.
 
 [source,xml]
 ----
 <memoryCircuitBreakerThresholdPct>75</memoryCircuitBreakerThresholdPct>
 ----
+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
+This circuit breaker tracks CPU utilization and triggers if the average CPU 
utilization over the last minute
+exceeds a configurable threshold.
+
+To enable/disable CPU utilization based circuit breaker, use the following 
configuration:
+
+[source,xml]
+----
+<isCpuCircuitBreakerEnabled>true</isCpuCircuitBreakerEnabled>
+----
+
+Note that this configuration will be overridden by the global circuit breaker 
flag -- if circuit breakers are disabled, this flag
+will not help you.
+
+To set the triggering threshold in units of CPU utilization, use the following 
parameter.
+[source,xml]
+----
+<cpuCircuitBreakerThreshold>75</cpuCircuitBreakerThreshold>
+----
+The range of valid values for this parameter is [40 to infinity].
+
+Note that both these circuit breakers are checked for each incoming search 
request and consider the current heap usage of the node and average CPU usage 
over the last minute, respectively.
+The checks does not impact performance, but any performance regressions that 
are suspected to be caused by this feature should be reported to the dev list.
 
 == Performance Considerations
-It is worth noting that while JVM circuit breaker does not add any noticeable 
overhead per query, having too many
+It is worth noting that while JVM or CPU circuit breakers does not add any 
noticeable overhead per query, having too many

Review comment:
       s/does not/do not

##########
File path: solr/solr-ref-guide/src/circuit-breakers.adoc
##########
@@ -35,33 +35,67 @@ will be disabled globally. Per circuit breaker 
configurations are specified in t
 <useCircuitBreakers>false</useCircuitBreakers>
 ----
 
+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.
+To enable/disable JVM heap usage based circuit breaker, use the following 
configuration:
+
+[source,xml]
+----
+<isMemoryCircuitBreakerEnabled>true</isMemoryCircuitBreakerEnabled>
+----
+
+Note that this configuration will be overridden by the global circuit breaker 
flag -- if circuit breakers are disabled, this flag
+will not help you.
+
+To set the triggering threshold as a percentage of the max heap allocated to 
the JVM, use the following parameter.
 
 [source,xml]
 ----
 <memoryCircuitBreakerThresholdPct>75</memoryCircuitBreakerThresholdPct>
 ----
+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
+This circuit breaker tracks CPU utilization and triggers if the average CPU 
utilization over the last minute
+exceeds a configurable threshold.
+
+To enable/disable CPU utilization based circuit breaker, use the following 
configuration:
+
+[source,xml]
+----
+<isCpuCircuitBreakerEnabled>true</isCpuCircuitBreakerEnabled>
+----
+
+Note that this configuration will be overridden by the global circuit breaker 
flag -- if circuit breakers are disabled, this flag
+will not help you.
+
+To set the triggering threshold in units of CPU utilization, use the following 
parameter.
+[source,xml]
+----
+<cpuCircuitBreakerThreshold>75</cpuCircuitBreakerThreshold>
+----
+The range of valid values for this parameter is [40 to infinity].

Review comment:
       Why is 40 a good lower bound?

##########
File path: solr/core/src/test/org/apache/solr/util/TestCircuitBreaker.java
##########
@@ -157,6 +163,57 @@ public void testBuildingMemoryPressure() {
     }
   }
 
+  public void testFakeCPUCircuitBreaker() {
+    ExecutorService executor = ExecutorUtil.newMDCAwareCachedThreadPool(
+        new SolrNamedThreadFactory("TestCircuitBreaker"));
+    HashMap<String, String> args = new HashMap<String, String>();
+
+    args.put(QueryParsing.DEFTYPE, CircuitBreaker.NAME);
+    args.put(CommonParams.FL, "id");
+
+    AtomicInteger failureCount = new AtomicInteger();
+
+    try {
+      removeAllExistingCircuitBreakers();
+
+      CircuitBreaker circuitBreaker = new 
FakeCPUCircuitBreaker(h.getCore().getSolrConfig());
+
+      h.getCore().getCircuitBreakerManager().register(circuitBreaker);
+
+      for (int i = 0; i < 5; i++) {
+        executor.submit(() -> {
+          try {
+            h.query(req("name:\"john smith\""));
+          } catch (SolrException e) {
+            if (!e.getMessage().startsWith("Circuit Breakers tripped")) {
+              if (log.isInfoEnabled()) {
+                String logMessage = "Expected error message for 
testFakeCPUCircuitBreaker was not received. Error message " + e.getMessage();
+                log.info(logMessage);
+              }
+              throw new RuntimeException("Expected error message was not 
received. Error message " + e.getMessage());
+            }
+            failureCount.incrementAndGet();
+          } catch (Exception e) {
+            throw new RuntimeException(e.getMessage());
+          }
+        });
+      }
+
+      executor.shutdown();
+      try {
+        executor.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS);
+      } catch (InterruptedException e) {
+        throw new RuntimeException(e.getMessage());

Review comment:
       thread.interrupt

##########
File path: solr/core/src/test/org/apache/solr/util/TestCircuitBreaker.java
##########
@@ -179,7 +236,13 @@ public void testResponseWithCBTiming() {
     );
   }
 
-  private class MockCircuitBreaker extends CircuitBreaker {
+  private void removeAllExistingCircuitBreakers() {
+    List<CircuitBreaker> registeredCircuitBreakers = 
h.getCore().getCircuitBreakerManager().getRegisteredCircuitBreakers();
+
+    registeredCircuitBreakers.clear();
+  }
+
+  private class MockCircuitBreaker extends MemoryCircuitBreaker {

Review comment:
       static class

##########
File path: solr/core/src/test/org/apache/solr/util/TestCircuitBreaker.java
##########
@@ -157,6 +163,57 @@ public void testBuildingMemoryPressure() {
     }
   }
 
+  public void testFakeCPUCircuitBreaker() {
+    ExecutorService executor = ExecutorUtil.newMDCAwareCachedThreadPool(
+        new SolrNamedThreadFactory("TestCircuitBreaker"));
+    HashMap<String, String> args = new HashMap<String, String>();
+
+    args.put(QueryParsing.DEFTYPE, CircuitBreaker.NAME);
+    args.put(CommonParams.FL, "id");
+
+    AtomicInteger failureCount = new AtomicInteger();
+
+    try {
+      removeAllExistingCircuitBreakers();
+
+      CircuitBreaker circuitBreaker = new 
FakeCPUCircuitBreaker(h.getCore().getSolrConfig());
+
+      h.getCore().getCircuitBreakerManager().register(circuitBreaker);
+
+      for (int i = 0; i < 5; i++) {
+        executor.submit(() -> {
+          try {
+            h.query(req("name:\"john smith\""));
+          } catch (SolrException e) {

Review comment:
       Does circuit breaker throw a more specific exception? It's kind of 
cumbersome from a client API perspective that they need to inspect the string 
message for knowing that they should retry from circuit breakers.




----------------------------------------------------------------
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:
us...@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org

Reply via email to