cpoerschke commented on a change in pull request #193:
URL: https://github.com/apache/solr/pull/193#discussion_r671290658
##########
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:
1/2: in the
https://github.com/apache/solr/pull/193/commits/09f9fe7ee4bd9e6b6b105e1e3fdf45fd288c07b8
commit on the pull request's branch I proposed the use of the `enable`
attribute based on that being the built-in one for PluginInfo as per
https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.9.0/solr/core/src/java/org/apache/solr/core/PluginInfo.java#L196-L199
##########
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:
2/2: Alternative wording might be this:
```suggestion
Each circuit breaker plugin can be independently enabled/disabled using the
`enable` attribute.
```
Or to remove the sentence and the `enable="true"` mentions in the examples
if/since by defaults plugins would presumably be enabled?
Another possibility would be
```
<circuitBreaker class="Foobar" enable="${circuitBreaker.foobar.enable:true}">
...
</circuitBreaker>
```
style examples on the (unconfirmed) assumption that this system property
usage is supported for these plugins, and this would be a way for users to
change the behaviour of an instance without a config change.
--
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
