You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/06/25 10:24:20 UTC
[GitHub] [solr] atris opened a new pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
atris opened a new pull request #193:
URL: https://github.com/apache/solr/pull/193
This commit introduces the capability of adding new circuit breakers on the fly by specifying the
circuit breaker and its parameters in solrconfig.xml.
Previously, it was possible to plug in new circuit breakers by plugging in a custom CircuitBreakerManager.
This commit makes it more elastic by allowing circuit breaker level pluggability.
CircuitBreakerConfig remains to support tests but is no longer used in the core code path.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #193:
URL: https://github.com/apache/solr/pull/193#discussion_r665691418
##########
File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java
##########
@@ -135,15 +125,61 @@ public static String toErrorMessage(List<CircuitBreaker> circuitBreakerList) {
*
* Any default circuit breakers should be registered here.
*/
+ @Deprecated
public static CircuitBreakerManager build(PluginInfo pluginInfo) {
+ return build(pluginInfo, null);
+ }
+
+ /**
+ * TODO
+ */
+ public static CircuitBreakerManager build(PluginInfo pluginInfo, SolrResourceLoader solrResourceLoader) {
boolean enabled = pluginInfo == null ? false : Boolean.parseBoolean(pluginInfo.attributes.getOrDefault("enabled", "false"));
CircuitBreakerManager circuitBreakerManager = new CircuitBreakerManager(enabled);
- circuitBreakerManager.init(pluginInfo);
+ circuitBreakerManager.init(pluginInfo, solrResourceLoader);
return circuitBreakerManager;
}
+ /**
+ * Initialize with circuit breakers defined in the configuration
+ */
+ public void init(PluginInfo pluginInfo, SolrResourceLoader solrResourceLoader) {
Review comment:
I have also given `ResourceLoaderAware` use a try but couldn't quite figure out how to make it work and if it's actually needed in this scenario? The https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.9.0/solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java#L823-L849 caused asserts to trigger if `SolrCoreAware` or `ResourceLoaderAware` is used and the comments guide against additions to the list (though admittedly I haven't yet looked at the SOLR-8311 referenced there).
`PluginInfoInitialized` use alone seems to be sufficient if we're happy to have the circuit breakers register with the circuit breaker manager similar to how the metric producers register with the metric manager?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #193:
URL: https://github.com/apache/solr/pull/193#discussion_r734099124
##########
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 would disagree on the proposed XML format
Is it the presence of an `eneable` attribute you don't like or the example variable substitution to make the attribute contollable from a sysProp? I think that was just an example from Chrstine of how things are often done in solr's config. But for this I doubt that it will even be added to the default solrconfig example at all? So let's just agree that the default is enabled and you can disable it with the xml attribute, and not with a child tag.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #193:
URL: https://github.com/apache/solr/pull/193#discussion_r666081011
##########
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>
+ <circuitBreaker class="org.apache.solr.util.circuitbreaker.CPUCircuitBreaker" enable="true">
+ <int name="threshold">75</int>
+ </circuitBreaker>
Review comment:
minor: keep existing indentation level
```suggestion
<circuitBreaker class="org.apache.solr.util.circuitbreaker.CPUCircuitBreaker" enable="true">
<int name="threshold">75</int>
</circuitBreaker>
```
##########
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:
Review comment:
post merge conflict resolving tweak:
```suggestion
All circuit breaker configurations are listed as independent `<circuitBreaker>` entries in `solrconfig.xml` as shown below:
```
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] atris commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
atris commented on a change in pull request #193:
URL: https://github.com/apache/solr/pull/193#discussion_r664377658
##########
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:
This isnt the CircuitBreakerManager being pluggable -- this is CircuitBreakerManager being the central point of configuration for all circuit breakers. So yes, circuit breakers are individually pluggable, and no, CBM is no longer pluggable. However, CBM still handles all the configuration management for circuit breakers, hence this registration
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] atris commented on pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
atris commented on pull request #193:
URL: https://github.com/apache/solr/pull/193#issuecomment-874666385
@cpoerschke Updated, let me know if it looks fine
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] atris commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
atris commented on a change in pull request #193:
URL: https://github.com/apache/solr/pull/193#discussion_r663656802
##########
File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java
##########
@@ -135,15 +125,61 @@ public static String toErrorMessage(List<CircuitBreaker> circuitBreakerList) {
*
* Any default circuit breakers should be registered here.
*/
+ @Deprecated
public static CircuitBreakerManager build(PluginInfo pluginInfo) {
+ return build(pluginInfo, null);
+ }
+
+ /**
+ * TODO
+ */
+ public static CircuitBreakerManager build(PluginInfo pluginInfo, SolrResourceLoader solrResourceLoader) {
boolean enabled = pluginInfo == null ? false : Boolean.parseBoolean(pluginInfo.attributes.getOrDefault("enabled", "false"));
CircuitBreakerManager circuitBreakerManager = new CircuitBreakerManager(enabled);
- circuitBreakerManager.init(pluginInfo);
+ circuitBreakerManager.init(pluginInfo, solrResourceLoader);
return circuitBreakerManager;
}
+ /**
+ * Initialize with circuit breakers defined in the configuration
+ */
+ public void init(PluginInfo pluginInfo, SolrResourceLoader solrResourceLoader) {
Review comment:
Yes, I did. The main challenge was that both (ResourceLoader and PluginInfo list) need to be present when CircuitBreakerManager can be initialised. If ResourceLoaderAware and PluginInfoInitialized are used, then CircuitBreakerManager will have to maintain state for both until they are both present, and then initialise itself. This seems a bit of an overkill?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] atris commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
atris commented on a change in pull request #193:
URL: https://github.com/apache/solr/pull/193#discussion_r664273166
##########
File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java
##########
@@ -49,18 +51,6 @@ public CircuitBreakerManager(final boolean enableCircuitBreakerManager) {
this.enableCircuitBreakerManager = enableCircuitBreakerManager;
}
- @Override
Review comment:
Yes, I will raise a separate PR for marking deprecation.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #193:
URL: https://github.com/apache/solr/pull/193#issuecomment-876313952
> ... This branch has conflicts that must be resolved ...
I'll have a go at resolving them.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] atris commented on pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
atris commented on pull request #193:
URL: https://github.com/apache/solr/pull/193#issuecomment-869413150
@janhoy @dsmiley @cpoerschke Please review
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #193:
URL: https://github.com/apache/solr/pull/193#discussion_r665688648
##########
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:
Thanks! Added commits, with hopefully more or less helpful commit messages. Let me know what you think.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] atris commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
atris commented on a change in pull request #193:
URL: https://github.com/apache/solr/pull/193#discussion_r673838104
##########
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 would disagree on the proposed XML format -- seems too clumsy to me. We could remove the enable flag altogether, although it exists for quick experimentation (toggle on/off) when using circuit breakers.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #193:
URL: https://github.com/apache/solr/pull/193#discussion_r664151764
##########
File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java
##########
@@ -135,15 +125,61 @@ public static String toErrorMessage(List<CircuitBreaker> circuitBreakerList) {
*
* Any default circuit breakers should be registered here.
*/
+ @Deprecated
public static CircuitBreakerManager build(PluginInfo pluginInfo) {
+ return build(pluginInfo, null);
+ }
+
+ /**
+ * TODO
+ */
+ public static CircuitBreakerManager build(PluginInfo pluginInfo, SolrResourceLoader solrResourceLoader) {
boolean enabled = pluginInfo == null ? false : Boolean.parseBoolean(pluginInfo.attributes.getOrDefault("enabled", "false"));
CircuitBreakerManager circuitBreakerManager = new CircuitBreakerManager(enabled);
- circuitBreakerManager.init(pluginInfo);
+ circuitBreakerManager.init(pluginInfo, solrResourceLoader);
return circuitBreakerManager;
}
+ /**
+ * Initialize with circuit breakers defined in the configuration
+ */
+ public void init(PluginInfo pluginInfo, SolrResourceLoader solrResourceLoader) {
Review comment:
I can see that. But perhaps worth giving it a try? The state machine could be quite simple. Each `inform()` would set an instance var and call `init()`, and `init()` would just exit if it lacks either or the two?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] atris commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
atris commented on a change in pull request #193:
URL: https://github.com/apache/solr/pull/193#discussion_r673916341
##########
File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java
##########
@@ -135,15 +125,61 @@ public static String toErrorMessage(List<CircuitBreaker> circuitBreakerList) {
*
* Any default circuit breakers should be registered here.
*/
+ @Deprecated
public static CircuitBreakerManager build(PluginInfo pluginInfo) {
+ return build(pluginInfo, null);
+ }
+
+ /**
+ * TODO
+ */
+ public static CircuitBreakerManager build(PluginInfo pluginInfo, SolrResourceLoader solrResourceLoader) {
boolean enabled = pluginInfo == null ? false : Boolean.parseBoolean(pluginInfo.attributes.getOrDefault("enabled", "false"));
CircuitBreakerManager circuitBreakerManager = new CircuitBreakerManager(enabled);
- circuitBreakerManager.init(pluginInfo);
+ circuitBreakerManager.init(pluginInfo, solrResourceLoader);
return circuitBreakerManager;
}
+ /**
+ * Initialize with circuit breakers defined in the configuration
+ */
+ public void init(PluginInfo pluginInfo, SolrResourceLoader solrResourceLoader) {
Review comment:
Agreed
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #193:
URL: https://github.com/apache/solr/pull/193#discussion_r665496641
##########
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:
Thanks for clarifying that circuit breakers are individually pluggable but the circuit breaker manager is no longer pluggable!
Would you be comfortable with me adding commits to the pull request branch? I've tried out experimentally to use `CircuitBreaker.class` instead of `CircuitBreakerManager.class` and also some other suggestions more easily shared in code instead of comment form e.g. use of the PluginInfo's built-in enable(d) 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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] atris commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
atris commented on a change in pull request #193:
URL: https://github.com/apache/solr/pull/193#discussion_r665674851
##########
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:
Absolutely!
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] atris commented on pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
atris commented on pull request #193:
URL: https://github.com/apache/solr/pull/193#issuecomment-873855553
@janhoy Updated the PR, please see if the approach looks fine to you.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] atris commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
atris commented on a change in pull request #193:
URL: https://github.com/apache/solr/pull/193#discussion_r663657042
##########
File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java
##########
@@ -49,18 +51,6 @@ public CircuitBreakerManager(final boolean enableCircuitBreakerManager) {
this.enableCircuitBreakerManager = enableCircuitBreakerManager;
}
- @Override
Review comment:
I was considering keeping this change only for Solr 9? The main challenge is that it is cumbersome to maintain two different formats of definition within the same (solrconfig.xml) file.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #193:
URL: https://github.com/apache/solr/pull/193#discussion_r664151041
##########
File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java
##########
@@ -49,18 +51,6 @@ public CircuitBreakerManager(final boolean enableCircuitBreakerManager) {
this.enableCircuitBreakerManager = enableCircuitBreakerManager;
}
- @Override
Review comment:
Ok, if this targets 9.0, then document it and we should be good. Will you then deprecate CircuitBreakerManager as a plugin in 8.10 and remove it in 9.0 in order to not confuse the two?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] atris commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
atris commented on a change in pull request #193:
URL: https://github.com/apache/solr/pull/193#discussion_r664273166
##########
File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java
##########
@@ -49,18 +51,6 @@ public CircuitBreakerManager(final boolean enableCircuitBreakerManager) {
this.enableCircuitBreakerManager = enableCircuitBreakerManager;
}
- @Override
Review comment:
Yes, I will raise a separate PR for marking deprecation.
##########
File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java
##########
@@ -135,15 +125,61 @@ public static String toErrorMessage(List<CircuitBreaker> circuitBreakerList) {
*
* Any default circuit breakers should be registered here.
*/
+ @Deprecated
public static CircuitBreakerManager build(PluginInfo pluginInfo) {
+ return build(pluginInfo, null);
+ }
+
+ /**
+ * TODO
+ */
+ public static CircuitBreakerManager build(PluginInfo pluginInfo, SolrResourceLoader solrResourceLoader) {
boolean enabled = pluginInfo == null ? false : Boolean.parseBoolean(pluginInfo.attributes.getOrDefault("enabled", "false"));
CircuitBreakerManager circuitBreakerManager = new CircuitBreakerManager(enabled);
- circuitBreakerManager.init(pluginInfo);
+ circuitBreakerManager.init(pluginInfo, solrResourceLoader);
return circuitBreakerManager;
}
+ /**
+ * Initialize with circuit breakers defined in the configuration
+ */
+ public void init(PluginInfo pluginInfo, SolrResourceLoader solrResourceLoader) {
Review comment:
@janhoy I am not sure if I understand the nuances of ResourceLoaderAware correctly, but have pushed an iteration. Please see and let me know if that is what you meant.
##########
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:
This isnt the CircuitBreakerManager being pluggable -- this is CircuitBreakerManager being the central point of configuration for all circuit breakers. So yes, circuit breakers are individually pluggable, and no, CBM is no longer pluggable. However, CBM still handles all the configuration management for circuit breakers, hence this registration
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #193:
URL: https://github.com/apache/solr/pull/193#discussion_r664151041
##########
File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java
##########
@@ -49,18 +51,6 @@ public CircuitBreakerManager(final boolean enableCircuitBreakerManager) {
this.enableCircuitBreakerManager = enableCircuitBreakerManager;
}
- @Override
Review comment:
Ok, if this targets 9.0, then document it and we should be good. Will you then deprecate CircuitBreakerManager as a plugin in 8.10 and remove it in 9.0 in order to not confuse the two?
##########
File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java
##########
@@ -135,15 +125,61 @@ public static String toErrorMessage(List<CircuitBreaker> circuitBreakerList) {
*
* Any default circuit breakers should be registered here.
*/
+ @Deprecated
public static CircuitBreakerManager build(PluginInfo pluginInfo) {
+ return build(pluginInfo, null);
+ }
+
+ /**
+ * TODO
+ */
+ public static CircuitBreakerManager build(PluginInfo pluginInfo, SolrResourceLoader solrResourceLoader) {
boolean enabled = pluginInfo == null ? false : Boolean.parseBoolean(pluginInfo.attributes.getOrDefault("enabled", "false"));
CircuitBreakerManager circuitBreakerManager = new CircuitBreakerManager(enabled);
- circuitBreakerManager.init(pluginInfo);
+ circuitBreakerManager.init(pluginInfo, solrResourceLoader);
return circuitBreakerManager;
}
+ /**
+ * Initialize with circuit breakers defined in the configuration
+ */
+ public void init(PluginInfo pluginInfo, SolrResourceLoader solrResourceLoader) {
Review comment:
I can see that. But perhaps worth giving it a try? The state machine could be quite simple. Each `inform()` would set an instance var and call `init()`, and `init()` would just exit if it lacks either or the two?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] atris commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
atris commented on a change in pull request #193:
URL: https://github.com/apache/solr/pull/193#discussion_r664292615
##########
File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java
##########
@@ -135,15 +125,61 @@ public static String toErrorMessage(List<CircuitBreaker> circuitBreakerList) {
*
* Any default circuit breakers should be registered here.
*/
+ @Deprecated
public static CircuitBreakerManager build(PluginInfo pluginInfo) {
+ return build(pluginInfo, null);
+ }
+
+ /**
+ * TODO
+ */
+ public static CircuitBreakerManager build(PluginInfo pluginInfo, SolrResourceLoader solrResourceLoader) {
boolean enabled = pluginInfo == null ? false : Boolean.parseBoolean(pluginInfo.attributes.getOrDefault("enabled", "false"));
CircuitBreakerManager circuitBreakerManager = new CircuitBreakerManager(enabled);
- circuitBreakerManager.init(pluginInfo);
+ circuitBreakerManager.init(pluginInfo, solrResourceLoader);
return circuitBreakerManager;
}
+ /**
+ * Initialize with circuit breakers defined in the configuration
+ */
+ public void init(PluginInfo pluginInfo, SolrResourceLoader solrResourceLoader) {
Review comment:
@janhoy I am not sure if I understand the nuances of ResourceLoaderAware correctly, but have pushed an iteration. Please see and let me know if that is what you meant.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #193:
URL: https://github.com/apache/solr/pull/193#issuecomment-949060142
I see you are awaiting my review on this. Would you mind bringing it up to date and resolve conversations that are settled, and then spell out where you need help to land this great improvement? I'll probably have some time next week to take the PR for a spin.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #193: SOLR-15474: Support Pluggable Circuit Breakers
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #193:
URL: https://github.com/apache/solr/pull/193#discussion_r659568848
##########
File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreaker.java
##########
@@ -33,16 +33,32 @@
public abstract class CircuitBreaker {
public static final String NAME = "circuitbreaker";
+ @Deprecated
protected final CircuitBreakerConfig config;
+ protected boolean enabled = false;
+
+ public CircuitBreaker() {
+ this.config = null;
+ }
+
+ @Deprecated
public CircuitBreaker(CircuitBreakerConfig config) {
this.config = config;
}
// Global config for all circuit breakers. For specific circuit breaker configs, define
// your own config.
protected boolean isEnabled() {
- return config.isEnabled();
+ if (config != null) {
Review comment:
can be simplified `return enabled || (config != null && config.isEnabled())`
##########
File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreaker.java
##########
@@ -33,16 +33,32 @@
public abstract class CircuitBreaker {
public static final String NAME = "circuitbreaker";
+ @Deprecated
protected final CircuitBreakerConfig config;
+ protected boolean enabled = false;
+
+ public CircuitBreaker() {
+ this.config = null;
+ }
+
+ @Deprecated
Review comment:
Consider adding `since` and `forRemoval` args to Deprecated annotations
##########
File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java
##########
@@ -49,18 +51,6 @@ public CircuitBreakerManager(final boolean enableCircuitBreakerManager) {
this.enableCircuitBreakerManager = enableCircuitBreakerManager;
}
- @Override
Review comment:
Isn't this a breaking change? If someone implemented their own circuitBreakerManager?
##########
File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java
##########
@@ -135,15 +125,61 @@ public static String toErrorMessage(List<CircuitBreaker> circuitBreakerList) {
*
* Any default circuit breakers should be registered here.
*/
+ @Deprecated
public static CircuitBreakerManager build(PluginInfo pluginInfo) {
+ return build(pluginInfo, null);
+ }
+
+ /**
+ * TODO
+ */
+ public static CircuitBreakerManager build(PluginInfo pluginInfo, SolrResourceLoader solrResourceLoader) {
boolean enabled = pluginInfo == null ? false : Boolean.parseBoolean(pluginInfo.attributes.getOrDefault("enabled", "false"));
CircuitBreakerManager circuitBreakerManager = new CircuitBreakerManager(enabled);
- circuitBreakerManager.init(pluginInfo);
+ circuitBreakerManager.init(pluginInfo, solrResourceLoader);
return circuitBreakerManager;
}
+ /**
+ * Initialize with circuit breakers defined in the configuration
+ */
+ public void init(PluginInfo pluginInfo, SolrResourceLoader solrResourceLoader) {
Review comment:
Did you consider `implements ResourceLoaderAware PluginInfoInitialized` annotations instead of hardwired init in CC?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org