cpoerschke commented on code in PR #1725:
URL: https://github.com/apache/solr/pull/1725#discussion_r1304176454
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java:
##########
@@ -36,26 +36,24 @@ public class MemoryCircuitBreaker extends CircuitBreaker {
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;
+ private long heapMemoryThreshold;
// Assumption -- the value of these parameters will be set correctly before
invoking
// getDebugInfo()
private static final ThreadLocal<Long> seenMemory =
ThreadLocal.withInitial(() -> 0L);
private static final ThreadLocal<Long> allowedMemory =
ThreadLocal.withInitial(() -> 0L);
- public MemoryCircuitBreaker(CircuitBreakerConfig config) {
- super(config);
-
- this.enabled = config.getMemCBEnabled();
+ public MemoryCircuitBreaker() {
+ super();
+ }
+ public void setThreshold(int thresholdValueInPercentage) {
Review Comment:
minor: `CPUCircuitBreaker` takes a `double` threshold, wondering if e.g. for
consistency the same might be done here? though staying with `int` is slightly
more backwards compatible in a way.
```suggestion
public void setThreshold(double thresholdValueInPercentage) {
```
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerRegistry.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.util.circuitbreaker;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Keeps track of all registered circuit breaker instances. Responsible for a
holistic view of
+ * whether a circuit breaker has tripped or not.
+ *
+ * <p>There are two typical ways of using this class's instance: 1. Check if
any circuit breaker has
+ * triggered -- and know which circuit breaker has triggered. 2. Get an
instance of a specific
+ * circuit breaker and perform checks.
+ *
+ * <p>It is a good practice to register new circuit breakers here if you want
them checked for every
+ * request.
Review Comment:
Registering (or not) here no longer is a choice i.e. everything in the
config is automatically registered here.
```suggestion
```
##########
solr/solr-ref-guide/modules/deployment-guide/pages/circuit-breakers.adoc:
##########
@@ -27,23 +27,16 @@ If circuit breakers are enabled, requests may be rejected
under the condition of
It is up to the client to handle this error and potentially build a retrial
logic as this should ideally be a transient situation.
== Circuit Breaker Configurations
-All circuit breaker configurations are listed in the `<circuitBreaker>` tags
in `solrconfig.xml` as shown below:
+All circuit breaker configurations are listed as independent
`<circuitBreaker>` entries in `solrconfig.xml` as shown below:
[source,xml]
----
-<circuitBreaker class="solr.CircuitBreakerManager" enabled="true">
- <!-- All specific configs in this section -->
+<circuitBreaker class="org.apache.solr.util.circuitbreaker.CPUCircuitBreaker">
+ <int name="threshold">75</int>
Review Comment:
```suggestion
<double name="threshold">75</double>
```
based on current `setThreshold` signature (though likely `int` could be used
with a `double` accessor but not vice versa)
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerRegistry.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.util.circuitbreaker;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Keeps track of all registered circuit breaker instances. Responsible for a
holistic view of
+ * whether a circuit breaker has tripped or not.
+ *
+ * <p>There are two typical ways of using this class's instance: 1. Check if
any circuit breaker has
+ * triggered -- and know which circuit breaker has triggered. 2. Get an
instance of a specific
+ * circuit breaker and perform checks.
+ *
+ * <p>It is a good practice to register new circuit breakers here if you want
them checked for every
+ * request.
+ *
+ * <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 CircuitBreakerRegistry {
+
+ private final List<CircuitBreaker> circuitBreakerList = new ArrayList<>();
+
+ public CircuitBreakerRegistry() {}
+
+ public void register(CircuitBreaker circuitBreaker) {
+ circuitBreakerList.add(circuitBreaker);
+ }
+
+ public void deregisterAll() {
+ circuitBreakerList.clear();
+ }
+ /**
+ * Check and return circuit breakers that have triggered
+ *
+ * @return CircuitBreakers which have triggered, null otherwise.
+ */
+ public List<CircuitBreaker> checkTripped() {
+ List<CircuitBreaker> triggeredCircuitBreakers = null;
+
+ for (CircuitBreaker circuitBreaker : circuitBreakerList) {
+ if (circuitBreaker.isTripped()) {
+ if (triggeredCircuitBreakers == null) {
+ triggeredCircuitBreakers = new ArrayList<>();
+ }
+
+ triggeredCircuitBreakers.add(circuitBreaker);
+ }
+ }
+
+ return triggeredCircuitBreakers;
+ }
+
+ /**
+ * Returns true if *any* circuit breaker has triggered, false if none have
triggered.
+ *
+ * <p>NOTE: This method short circuits the checking of circuit breakers --
the method will return
+ * as soon as it finds a circuit breaker that is enabled and has triggered.
Review Comment:
there now no longer is a concept of a not-enabled circuit breaker
```suggestion
* as soon as it finds a circuit breaker that has triggered.
```
##########
solr/solr-ref-guide/modules/deployment-guide/pages/circuit-breakers.adoc:
##########
@@ -52,11 +45,13 @@ For using specific circuit breakers, each one needs to be
individually enabled i
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.
-Configuration for JVM heap usage based circuit breaker:
+To enable and configure the JVM heap usage based circuit breaker, add the
following:
[source,xml]
----
-<str name="memEnabled">true</str>
+<circuitBreaker
class="org.apache.solr.util.circuitbreaker.MemoryCircuitBreaker">
+ <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.
Review Comment:
```suggestion
```
##########
solr/solr-ref-guide/modules/deployment-guide/pages/circuit-breakers.adoc:
##########
@@ -66,7 +61,7 @@ It is controlled by the below configuration:
[source,xml]
----
-<str name="memThreshold">75</str>
+<int name="threshold">75</int>
----
Review Comment:
I think this block and the `It is controlled by the below configuration:`
line above can be removed now since the earlier block includes it.
##########
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">
+ <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>
----
Review Comment:
Suggest to remove/omit.
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java:
##########
@@ -17,163 +17,96 @@
package org.apache.solr.util.circuitbreaker;
-import com.google.common.annotations.VisibleForTesting;
-import java.util.ArrayList;
-import java.util.List;
+import java.lang.invoke.MethodHandles;
import org.apache.solr.common.util.NamedList;
-import org.apache.solr.core.PluginInfo;
-import org.apache.solr.util.plugin.PluginInfoInitialized;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
- * Manages all registered circuit breaker instances. Responsible for a
holistic view of whether a
- * circuit breaker has tripped or not.
+ * Single CB that registers both Mem and CPU CB. This is only for backward
compatibility with the
+ * 9.x versions prior to 9.4.
*
- * <p>There are two typical ways of using this class's instance: 1. Check if
any circuit breaker has
- * triggered -- and know which circuit breaker has triggered. 2. Get an
instance of a specific
- * circuit breaker and perform checks.
- *
- * <p>It is a good practice to register new circuit breakers here if you want
them checked for every
- * request.
- *
- * <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.
+ * @deprecated Use individual Circuit Breakers instead
*/
-public class CircuitBreakerManager implements PluginInfoInitialized {
- // Class private to potentially allow "family" of circuit breakers to be
enabled or disabled
- private final boolean enableCircuitBreakerManager;
-
- private final List<CircuitBreaker> circuitBreakerList = new ArrayList<>();
-
- public CircuitBreakerManager(final boolean enableCircuitBreakerManager) {
- this.enableCircuitBreakerManager = enableCircuitBreakerManager;
+@Deprecated(since = "9.4")
+public class CircuitBreakerManager extends CircuitBreaker {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private boolean cpuEnabled;
+ private boolean memEnabled;
+ private int memThreshold = 100;
+ private int cpuThreshold = 100;
+ private MemoryCircuitBreaker memCB;
+ private CPUCircuitBreaker cpuCB;
+
+ public CircuitBreakerManager() {
+ super();
}
@Override
- public void init(PluginInfo pluginInfo) {
- CircuitBreaker.CircuitBreakerConfig circuitBreakerConfig =
buildCBConfig(pluginInfo);
-
- // Install the default circuit breakers
- CircuitBreaker memoryCircuitBreaker = new
MemoryCircuitBreaker(circuitBreakerConfig);
- CircuitBreaker cpuCircuitBreaker = new
CPUCircuitBreaker(circuitBreakerConfig);
-
- register(memoryCircuitBreaker);
- register(cpuCircuitBreaker);
- }
-
- public void register(CircuitBreaker circuitBreaker) {
- circuitBreakerList.add(circuitBreaker);
+ public boolean isTripped() {
+ return (memEnabled && memCB.isTripped()) || (cpuEnabled &&
cpuCB.isTripped());
}
- public void deregisterAll() {
- circuitBreakerList.clear();
- }
- /**
- * Check and return circuit breakers that have triggered
- *
- * @return CircuitBreakers which have triggered, null otherwise.
- */
- public List<CircuitBreaker> checkTripped() {
- List<CircuitBreaker> triggeredCircuitBreakers = null;
-
- if (enableCircuitBreakerManager) {
- for (CircuitBreaker circuitBreaker : circuitBreakerList) {
- if (circuitBreaker.isEnabled() && circuitBreaker.isTripped()) {
- if (triggeredCircuitBreakers == null) {
- triggeredCircuitBreakers = new ArrayList<>();
- }
-
- triggeredCircuitBreakers.add(circuitBreaker);
- }
- }
+ @Override
+ public String getDebugInfo() {
+ StringBuilder sb = new StringBuilder();
+ if (memEnabled) {
+ sb.append(memCB.getDebugInfo()).append("\n");
}
-
- return triggeredCircuitBreakers;
- }
-
- /**
- * Returns true if *any* circuit breaker has triggered, false if none have
triggered.
- *
- * <p>NOTE: This method short circuits the checking of circuit breakers --
the method will return
- * as soon as it finds a circuit breaker that is enabled and has triggered.
- */
- public boolean checkAnyTripped() {
- if (enableCircuitBreakerManager) {
- for (CircuitBreaker circuitBreaker : circuitBreakerList) {
- if (circuitBreaker.isEnabled() && circuitBreaker.isTripped()) {
- return true;
- }
- }
+ if (cpuEnabled) {
+ sb.append(cpuCB.getDebugInfo());
}
-
- return false;
+ return sb.toString();
}
- /**
- * Construct the final error message to be printed when circuit breakers
trip.
- *
- * @param circuitBreakerList Input list for circuit breakers.
- * @return Constructed error message.
- */
- public static String toErrorMessage(List<CircuitBreaker> circuitBreakerList)
{
+ @Override
+ public String getErrorMessage() {
StringBuilder sb = new StringBuilder();
-
- for (CircuitBreaker circuitBreaker : circuitBreakerList) {
- sb.append(circuitBreaker.getErrorMessage());
- sb.append("\n");
+ if (memEnabled) {
+ sb.append(memCB.getErrorMessage()).append("\n");
+ }
+ if (cpuEnabled) {
+ sb.append(cpuCB.getErrorMessage());
}
Review Comment:
```suggestion
if (memEnabled) {
sb.append(memCB.getErrorMessage());
}
if (memEnabled && cpuEnabled) {
sb.append("\n");
}
if (cpuEnabled) {
sb.append(cpuCB.getErrorMessage());
}
```
##########
solr/core/src/test-files/solr/collection1/conf/solrconfig-legacy-circuitbreaker.xml:
##########
@@ -78,11 +78,14 @@
</query>
- <circuitBreaker class="solr.CircuitBreakerManager" enabled="true">
+ <!-- Legacy config used in 9.x.
+ TODO: Remove this file in 10.0
+ -->
+ <circuitBreaker class="solr.CircuitBreakerManager">
<str name="memEnabled">true</str>
- <str name="memThreshold">75</str>
+ <str name="memThreshold">80</str>
<str name="cpuEnabled">true</str>
- <str name="cpuThreshold">75</str>
+ <str name="cpuThreshold">80</str>
Review Comment:
minor: could we retain `75` thresholds here? that also being what
`solrconfig-pluggable-circuitbreaker.xml` uses?
##########
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">
+ <int name="threshold">75</int>
Review Comment:
```suggestion
<double name="threshold">75</double>
```
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerRegistry.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.util.circuitbreaker;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Keeps track of all registered circuit breaker instances. Responsible for a
holistic view of
+ * whether a circuit breaker has tripped or not.
+ *
+ * <p>There are two typical ways of using this class's instance: 1. Check if
any circuit breaker has
+ * triggered -- and know which circuit breaker has triggered. 2. Get an
instance of a specific
+ * circuit breaker and perform checks.
+ *
+ * <p>It is a good practice to register new circuit breakers here if you want
them checked for every
+ * request.
+ *
+ * <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.
Review Comment:
Not sure if this note is still applicable or if not having it would be
clearer.
```suggestion
```
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerRegistry.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.util.circuitbreaker;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Keeps track of all registered circuit breaker instances. Responsible for a
holistic view of
+ * whether a circuit breaker has tripped or not.
+ *
+ * <p>There are two typical ways of using this class's instance: 1. Check if
any circuit breaker has
+ * triggered -- and know which circuit breaker has triggered. 2. Get an
instance of a specific
+ * circuit breaker and perform checks.
+ *
+ * <p>It is a good practice to register new circuit breakers here if you want
them checked for every
+ * request.
+ *
+ * <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 CircuitBreakerRegistry {
+
+ private final List<CircuitBreaker> circuitBreakerList = new ArrayList<>();
+
+ public CircuitBreakerRegistry() {}
+
+ public void register(CircuitBreaker circuitBreaker) {
+ circuitBreakerList.add(circuitBreaker);
+ }
+
+ public void deregisterAll() {
Review Comment:
```suggestion
@VisibleForTesting
public void deregisterAll() {
```
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java:
##########
@@ -17,163 +17,96 @@
package org.apache.solr.util.circuitbreaker;
-import com.google.common.annotations.VisibleForTesting;
-import java.util.ArrayList;
-import java.util.List;
+import java.lang.invoke.MethodHandles;
import org.apache.solr.common.util.NamedList;
-import org.apache.solr.core.PluginInfo;
-import org.apache.solr.util.plugin.PluginInfoInitialized;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
- * Manages all registered circuit breaker instances. Responsible for a
holistic view of whether a
- * circuit breaker has tripped or not.
+ * Single CB that registers both Mem and CPU CB. This is only for backward
compatibility with the
Review Comment:
```suggestion
* Single CircuitBreaker that registers both a Memory and a CPU
CircuitBreaker. This is only for backward compatibility with the
```
##########
solr/solr-ref-guide/modules/deployment-guide/pages/circuit-breakers.adoc:
##########
@@ -27,23 +27,16 @@ If circuit breakers are enabled, requests may be rejected
under the condition of
It is up to the client to handle this error and potentially build a retrial
logic as this should ideally be a transient situation.
== Circuit Breaker Configurations
-All circuit breaker configurations are listed in the `<circuitBreaker>` tags
in `solrconfig.xml` as shown below:
+All circuit breaker configurations are listed as independent
`<circuitBreaker>` entries in `solrconfig.xml` as shown below:
[source,xml]
----
-<circuitBreaker class="solr.CircuitBreakerManager" enabled="true">
- <!-- All specific configs in this section -->
+<circuitBreaker class="org.apache.solr.util.circuitbreaker.CPUCircuitBreaker">
+ <int name="threshold">75</int>
</circuitBreaker>
----
-The `enabled` attribute controls the global activation/deactivation of circuit
breakers.
-If this flag is disabled, all circuit breakers will be disabled globally.
-Per circuit breaker configurations are specified in their respective sections
later.
-
-This attribute 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.
-
-`CircuitBreakerManager` is the default manager for all circuit breakers that
should be defined in the tag unless the user wishes to use a custom
implementation.
+Each circuit breaker can be independently enabled/disabled using the
corresponding flag.
Review Comment:
How about having an example for this (or to remove the sentence)? E.g. to
clarify if `<circuitBreaker
class="org.apache.solr.util.circuitbreaker.CPUCircuitBreaker" enable="false">`
or `<circuitBreaker
class="org.apache.solr.util.circuitbreaker.CPUCircuitBreaker" enabled="false">`
or `<bool name="enabled">false</bool>` or something else is the usage.
--
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
