cpoerschke commented on code in PR #1871:
URL: https://github.com/apache/solr/pull/1871#discussion_r1315990360
##########
solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java:
##########
@@ -330,8 +331,8 @@ protected ResponseBuilder newResponseBuilder(
}
/**
- * Check if circuit breakers are tripped. Override this method in sub
classes that do not want to
- * check circuit breakers.
+ * Check if SEARCH circuit breakers are tripped. Override this method in sub
classes that do not
Review Comment:
```suggestion
* Check if {@link SolrRequestType#QUERY} circuit breakers are tripped.
Override this method in sub classes that do not
```
##########
solr/solr-ref-guide/modules/deployment-guide/pages/circuit-breakers.adoc:
##########
@@ -72,6 +73,30 @@ To enable and configure the CPU utilization based circuit
breaker:
The `threshold` is defined in units of CPU utilization.
+== Advanced example
+
+In this example we will prevent update requests above 80% CPU load, and
prevent query requests above 95% CPU load. Supported request types are `query`
and `update`.
+This would prevent expensive bulk updates from impacting search. Note also the
support for short-form class name.
+
+[source,xml]
+----
+<config>
+ <circuitBreaker class="solr.CPUCircuitBreaker">
+ <double name="threshold">80</double>
+ <arr name="requestTypes">
+ <str>update</str>
+ </arr>
+ </circuitBreaker>
+
+ <circuitBreaker class="solr.CPUCircuitBreaker">
+ <double name="threshold">95</double>
+ <arr name="requestTypes">
+ <str>query</str>
+ </arr>
+ </circuitBreaker>
+</config>
+----
+
== Performance Considerations
It is worth noting that while JVM or CPU circuit breakers do not add any
noticeable overhead per query, having too many circuit breakers checked for a
single request can cause a performance overhead.
Review Comment:
```suggestion
It is worth noting that while JVM or CPU circuit breakers do not add any
noticeable overhead per request, having too many circuit breakers checked for a
single request can cause a performance overhead.
```
##########
solr/core/src/test/org/apache/solr/util/BaseTestCircuitBreaker.java:
##########
@@ -79,21 +80,42 @@ public void testCBAlwaysTrips() {
});
}
- public void testCBFakeMemoryPressure() {
+ public void testCBFakeMemoryPressure() throws Exception {
removeAllExistingCircuitBreakers();
- CircuitBreaker circuitBreaker = new FakeMemoryPressureCircuitBreaker();
- MemoryCircuitBreaker memoryCircuitBreaker = (MemoryCircuitBreaker)
circuitBreaker;
+ // Update and search will not trip
Review Comment:
```suggestion
// Update and query will not trip
```
##########
solr/core/src/test/org/apache/solr/util/BaseTestCircuitBreaker.java:
##########
@@ -79,21 +80,42 @@ public void testCBAlwaysTrips() {
});
}
- public void testCBFakeMemoryPressure() {
+ public void testCBFakeMemoryPressure() throws Exception {
removeAllExistingCircuitBreakers();
- CircuitBreaker circuitBreaker = new FakeMemoryPressureCircuitBreaker();
- MemoryCircuitBreaker memoryCircuitBreaker = (MemoryCircuitBreaker)
circuitBreaker;
+ // Update and search will not trip
+ h.update(
+ "<add><doc><field name=\"id\">1</field><field name=\"name\">john
smith</field></doc></add>");
+ h.query(req("name:\"john smith\""));
- memoryCircuitBreaker.setThreshold(75);
+ MemoryCircuitBreaker searchBreaker = new
FakeMemoryPressureCircuitBreaker();
+ searchBreaker.setThreshold(80);
+ // Default request type is "search"
+ // searchBreaker.setRequestTypes(List.of("search"));
+ h.getCore().getCircuitBreakerRegistry().register(searchBreaker);
- h.getCore().getCircuitBreakerRegistry().register(circuitBreaker);
+ // Search will trip, but not update due to defaults
Review Comment:
```suggestion
// Query will trip, but not update due to defaults
```
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreaker.java:
##########
@@ -52,4 +62,48 @@ public CircuitBreaker() {}
/** Get error message when the circuit breaker triggers */
public abstract String getErrorMessage();
+
+ /**
+ * Set the request types for which this circuit breaker should be checked.
If not called, the
+ * circuit breaker will be checked for the {@link SolrRequestType#QUERY}
request type only.
+ *
+ * @param requestTypes list of strings representing request types
+ * @throws IllegalArgumentException if the request type is not valid
+ */
+ public void setRequestTypes(List<String> requestTypes) {
+ this.requestTypes =
+ requestTypes.stream()
+ .map(t -> SolrRequestType.valueOf(t.toUpperCase(Locale.ROOT)))
+ .peek(
+ t -> {
+ if (!SUPPORTED_TYPES.contains(t)) {
+ throw new IllegalArgumentException(
+ String.format(
+ Locale.ROOT,
+ "Request type %s is not supported for circuit
breakers",
+ t.name()));
+ }
+ })
+ .collect(Collectors.toSet());
+ }
+
+ public Set<SolrRequestType> getRequestTypes() {
+ return requestTypes;
+ }
+
+ /**
+ * Return the proper error code to use in exception. For legacy use of
{@link CircuitBreaker} we
+ * return 503 for backward compatibility, else return 429.
+ *
+ * @deprecated Remove in 10.0
+ */
+ @Deprecated(since = "9.4")
+ public static SolrException.ErrorCode getErrorCode(List<CircuitBreaker>
trippedCircuitBreakers) {
+ if (trippedCircuitBreakers != null
+ && trippedCircuitBreakers.stream().anyMatch(cb -> cb instanceof
CircuitBreakerManager)) {
Review Comment:
This probably would be over-engineering but conceptually we could give the
(deprecated) `CircuitBreakerManager` an error code configurable field,
defaulted to existing behaviour. Users could then choose to transition over to
the new return code separately from transitioning to the new circuit breakers.
```
CircuitBreakerManager cbm = (CircuitBreakerManager) cb;
return cbm.getErrorCode();
```
##########
solr/CHANGES.txt:
##########
@@ -72,6 +72,8 @@ New Features
---------------------
* SOLR-16654: Add support for node-level caches (Michael Gibney)
+* SOLR-16954: Make Circuit Breakers available for Update Requests (janhoy,
Christine Poerschke)
Review Comment:
```suggestion
* SOLR-16954: Make Circuit Breakers available for Update Requests (janhoy,
Christine Poerschke, Pierre Salagnac)
```
##########
solr/core/src/test/org/apache/solr/util/BaseTestCircuitBreaker.java:
##########
@@ -79,21 +80,42 @@ public void testCBAlwaysTrips() {
});
}
- public void testCBFakeMemoryPressure() {
+ public void testCBFakeMemoryPressure() throws Exception {
removeAllExistingCircuitBreakers();
- CircuitBreaker circuitBreaker = new FakeMemoryPressureCircuitBreaker();
- MemoryCircuitBreaker memoryCircuitBreaker = (MemoryCircuitBreaker)
circuitBreaker;
+ // Update and search will not trip
+ h.update(
+ "<add><doc><field name=\"id\">1</field><field name=\"name\">john
smith</field></doc></add>");
+ h.query(req("name:\"john smith\""));
- memoryCircuitBreaker.setThreshold(75);
+ MemoryCircuitBreaker searchBreaker = new
FakeMemoryPressureCircuitBreaker();
+ searchBreaker.setThreshold(80);
+ // Default request type is "search"
+ // searchBreaker.setRequestTypes(List.of("search"));
Review Comment:
```suggestion
// Default request type is "query"
// searchBreaker.setRequestTypes(List.of("query"));
```
##########
solr/core/src/java/org/apache/solr/handler/ContentStreamHandlerBase.java:
##########
@@ -101,6 +112,30 @@ public void handleRequestBody(SolrQueryRequest req,
SolrQueryResponse rsp) throw
}
}
+ /**
+ * Check if UPDATE circuit breakers are tripped. Override this method in sub
classes that do not
Review Comment:
```suggestion
* Check if {@link SolrRequestType#UPDATE} circuit breakers are tripped.
Override this method in sub classes that do not
```
##########
solr/solr-ref-guide/modules/deployment-guide/pages/circuit-breakers.adoc:
##########
@@ -22,18 +22,19 @@ resource configuration.
== When To Use Circuit Breakers
Circuit breakers should be used when the user wishes to trade request
throughput for a higher Solr stability.
-If circuit breakers are enabled, requests may be rejected under the condition
of high node duress with an appropriate HTTP error code (typically 503).
+If circuit breakers are enabled, requests may be rejected under the condition
of high node duress with HTTP error code 429 'Too Many Requests'.
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 as independent
`<circuitBreaker>` entries in `solrconfig.xml` as shown below.
+A circuit breaker can register itself to trip for search requests and/or
update requests. By default only search requests are affected. A user may
register multiple circuit breakers of the same type with different thresholds
for each request type.
Review Comment:
```suggestion
A circuit breaker can register itself to trip for query requests and/or
update requests. By default only search requests are affected. A user may
register multiple circuit breakers of the same type with different thresholds
for each request type.
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
