madrob commented on a change in pull request #193:
URL: https://github.com/apache/solr/pull/193#discussion_r670796347
##########
File path: solr/solr-ref-guide/src/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"
enable="true">
+ <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:
I'm confused about this because it looks like we don't read the flag
anywhere anymore.
##########
File path:
solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java
##########
@@ -39,26 +36,11 @@
* NOTE: The current way of registering new default circuit breakers is
minimal and not a long term
Review comment:
Do we need to update this comment?
##########
File path: solr/core/src/test/org/apache/solr/util/TestCircuitBreaker.java
##########
@@ -114,10 +108,10 @@ public void testCBFakeMemoryPressure() {
removeAllExistingCircuitBreakers();
- PluginInfo pluginInfo =
h.getCore().getSolrConfig().getPluginInfo(CircuitBreakerManager.class.getName());
+ CircuitBreaker circuitBreaker = new FakeMemoryPressureCircuitBreaker();
+ MemoryCircuitBreaker memoryCircuitBreaker = (MemoryCircuitBreaker)
circuitBreaker;
Review comment:
Declare circuitBreaker as a MemoryCircuitBreaker so we don't need this
cast. Same pattern below too.
##########
File path:
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:
do we need enabled=true here?
--
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
