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