cpoerschke commented on a change in pull request #193:
URL: https://github.com/apache/solr/pull/193#discussion_r664361017
##########
File path: solr/core/src/java/org/apache/solr/core/SolrConfig.java
##########
@@ -743,7 +743,16 @@ public PluginInfo getPluginInfo(String type) {
"Multiple plugins configured for type: " + type);
}
- private void initLibs(SolrResourceLoader loader, boolean isConfigsetTrusted)
{
+ public List<PluginInfo> getAllPluginInfos(String type) {
+ List<PluginInfo> result = pluginStore.get(type);
+ if (result == null || result.isEmpty()) {
+ return null;
+ }
+
+ return result;
+ }
+
+ private void initLibs(SolrResourceLoader loader, boolean
isConfigsetTrusted) {
Review comment:
minor
```suggestion
private void initLibs(SolrResourceLoader loader, boolean
isConfigsetTrusted) {
```
##########
File path:
solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java
##########
@@ -39,26 +45,21 @@
* 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 {
+public class CircuitBreakerManager implements ResourceLoaderAware,
PluginInfoInitialized {
Review comment:
question 2 of 2: If (and that is an if) the circuit breaker manager no
longer needs to be configurable then perhaps the ` PluginInfoInitialized` no
longer needs to be implemented here?
```suggestion
public class CircuitBreakerManager implements ResourceLoaderAware {
```
##########
File path: solr/core/src/java/org/apache/solr/core/SolrConfig.java
##########
@@ -357,7 +357,7 @@ public static final Version parseLuceneVersionString(final
String matchVersion)
.add(new SolrPluginInfo(IndexSchemaFactory.class, "schemaFactory",
REQUIRE_CLASS))
.add(new SolrPluginInfo(RestManager.class, "restManager"))
.add(new SolrPluginInfo(StatsCache.class, "statsCache", REQUIRE_CLASS))
- .add(new SolrPluginInfo(CircuitBreakerManager.class, "circuitBreaker"))
+ .add(new SolrPluginInfo(CircuitBreakerManager.class, "circuitBreaker",
MULTI_OK))
Review comment:
question 1 of 2: If circuit breakers are individually pluggable, is
there still a requirement for the circuit breaker manager to be configurable
too?
```suggestion
.add(new SolrPluginInfo(CircuitBreaker.class, "circuitBreaker",
MULTI_OK))
```
##########
File path: solr/solr-ref-guide/src/circuit-breakers.adoc
##########
@@ -107,3 +104,14 @@ 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" enabled="true">
+ <bool name="enabled">true</bool>
+ <int name="threshold">75</int>
+</circuitBreaker>
Review comment:
```suggestion
<circuitBreaker class="foobar" enabled="true">
<int name="threshold">75</int>
</circuitBreaker>
```
I wonder if the `enabled` flag could be removed in favour of the `enabled`
attribute?
--
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
