cpoerschke commented on code in PR #1725: URL: https://github.com/apache/solr/pull/1725#discussion_r1243853319
########## solr/core/src/java/org/apache/solr/util/circuitbreaker/CPUCircuitBreaker.java: ########## @@ -38,31 +38,24 @@ public class CPUCircuitBreaker extends CircuitBreaker { private static final OperatingSystemMXBean operatingSystemMXBean = ManagementFactory.getOperatingSystemMXBean(); - private final boolean enabled; - private final double cpuUsageThreshold; + private double cpuUsageThreshold; // Assumption -- the value of these parameters will be set correctly before invoking // getDebugInfo() private static final ThreadLocal<Double> seenCPUUsage = ThreadLocal.withInitial(() -> 0.0); private static final ThreadLocal<Double> allowedCPUUsage = ThreadLocal.withInitial(() -> 0.0); - public CPUCircuitBreaker(CircuitBreakerConfig config) { - super(config); + public CPUCircuitBreaker() { + super(); + } - this.enabled = config.getCpuCBEnabled(); - this.cpuUsageThreshold = config.getCpuCBThreshold(); + public void setThreshold(double usageThreshold) { + this.cpuUsageThreshold = usageThreshold; Review Comment: ```suggestion public void setThreshold(double threshold) { this.cpuUsageThreshold = threshold; ``` ########## solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java: ########## @@ -38,27 +35,11 @@ * <p>NOTE: The current way of registering new default circuit breakers is minimal and not a long * term solution. There will be a follow up with a SIP for a schema API design. */ -public class CircuitBreakerManager implements PluginInfoInitialized { - // Class private to potentially allow "family" of circuit breakers to be enabled or disabled - private final boolean enableCircuitBreakerManager; +public class CircuitBreakerManager { Review Comment: Wondering if the last two paragraphs of the class-level javadoc can be omitted or replaced now then and perhaps the class could even be marked final to make it clear that extending it is no longer intendeds? ```suggestion public final class CircuitBreakerManager { ``` ########## solr/solr-ref-guide/modules/deployment-guide/pages/circuit-breakers.adoc: ########## @@ -84,25 +79,35 @@ This circuit breaker tracks CPU utilization and triggers if the average CPU util Note that the value used in computation is over the last one minute -- so a sudden spike in traffic that goes down might still cause the circuit breaker to trigger for a short while before it resolves and updates the value. For more details of the calculation, please see https://en.wikipedia.org/wiki/Load_(computing) -Configuration for CPU utilization based circuit breaker: +To enable and configure the CPU utilization based circuit breaker: [source,xml] ---- -<str name="cpuEnabled">true</str> +<circuitBreaker class="org.apache.solr.util.circuitbreaker.CPUCircuitBreaker" enable="true"> + <int name="threshold">75</int> +</circuitBreaker> ---- -Note that this configuration will be overridden by the global circuit breaker flag -- if circuit breakers are disabled, this flag will not help you. - The triggering threshold is defined in units of CPU utilization. The configuration to control this is as below: [source,xml] ---- -<str name="cpuThreshold">75</str> +<int name="threshold">75</int> ---- == Performance Considerations It is worth noting that while JVM or CPU circuit breakers do not add any noticeable overhead per query, having too many circuit breakers checked for a single request can cause a performance overhead. In addition, it is a good practice to exponentially back off while retrying requests on a busy node. + +== Adding New Circuit Breakers +To add new circuit breakers, add the new circuit breaker's class and required parameters: + +[source,xml] +---- +<circuitBreaker class="foobar" enable="true"> Review Comment: ```suggestion <circuitBreaker class="com.mycompany.solr.FooBarCircuitBreaker" enable="true"> ``` ########## solr/core/src/test-files/solr/collection1/conf/solrconfig-memory-circuitbreaker.xml: ########## @@ -78,11 +78,12 @@ </query> - <circuitBreaker class="solr.CircuitBreakerManager" enabled="true"> - <str name="memEnabled">true</str> - <str name="memThreshold">75</str> - <str name="cpuEnabled">true</str> - <str name="cpuThreshold">75</str> + <circuitBreaker class="org.apache.solr.util.circuitbreaker.MemoryCircuitBreaker" enable="true"> + <int name="threshold">75</int> + </circuitBreaker> + + <circuitBreaker class="org.apache.solr.util.circuitbreaker.CPUCircuitBreaker" enable="true"> + <int name="threshold">75</int> Review Comment: Not sure if the `int` might need to be non-`int` here to match the `setThreshold(double threshold)` signature e.g. ```suggestion <double name="threshold">75</double> ``` ########## solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreaker.java: ########## @@ -29,20 +33,15 @@ * <p>The class and its derivatives raise a standard exception when a circuit breaker is triggered. * We should make it into a dedicated exception (https://issues.apache.org/jira/browse/SOLR-14755) */ -public abstract class CircuitBreaker { +public abstract class CircuitBreaker implements NamedListInitializedPlugin { public static final String NAME = "circuitbreaker"; Review Comment: ```suggestion ``` ########## solr/core/src/test-files/solr/collection1/conf/solrconfig-memory-circuitbreaker.xml: ########## @@ -78,11 +78,12 @@ </query> - <circuitBreaker class="solr.CircuitBreakerManager" enabled="true"> - <str name="memEnabled">true</str> - <str name="memThreshold">75</str> - <str name="cpuEnabled">true</str> - <str name="cpuThreshold">75</str> + <circuitBreaker class="org.apache.solr.util.circuitbreaker.MemoryCircuitBreaker" enable="true"> Review Comment: ```suggestion <circuitBreaker class="org.apache.solr.util.circuitbreaker.MemoryCircuitBreaker"> ``` ########## solr/core/src/test-files/solr/collection1/conf/solrconfig-memory-circuitbreaker.xml: ########## @@ -78,11 +78,12 @@ </query> - <circuitBreaker class="solr.CircuitBreakerManager" enabled="true"> - <str name="memEnabled">true</str> - <str name="memThreshold">75</str> - <str name="cpuEnabled">true</str> - <str name="cpuThreshold">75</str> + <circuitBreaker class="org.apache.solr.util.circuitbreaker.MemoryCircuitBreaker" enable="true"> + <int name="threshold">75</int> + </circuitBreaker> + + <circuitBreaker class="org.apache.solr.util.circuitbreaker.CPUCircuitBreaker" enable="true"> Review Comment: ```suggestion <circuitBreaker class="org.apache.solr.util.circuitbreaker.CPUCircuitBreaker"> ``` ########## solr/core/src/java/org/apache/solr/util/circuitbreaker/CPUCircuitBreaker.java: ########## @@ -38,31 +38,24 @@ public class CPUCircuitBreaker extends CircuitBreaker { private static final OperatingSystemMXBean operatingSystemMXBean = ManagementFactory.getOperatingSystemMXBean(); - private final boolean enabled; - private final double cpuUsageThreshold; + private double cpuUsageThreshold; // Assumption -- the value of these parameters will be set correctly before invoking // getDebugInfo() private static final ThreadLocal<Double> seenCPUUsage = ThreadLocal.withInitial(() -> 0.0); private static final ThreadLocal<Double> allowedCPUUsage = ThreadLocal.withInitial(() -> 0.0); - public CPUCircuitBreaker(CircuitBreakerConfig config) { - super(config); + public CPUCircuitBreaker() { + super(); + } Review Comment: minor: not 100% sure if this is needed (been doing more non-Java code recently ...) ```suggestion ``` -- 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. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org