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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
