You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "janhoy (via GitHub)" <gi...@apache.org> on 2023/08/29 12:42:11 UTC
[GitHub] [solr] janhoy opened a new pull request, #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
janhoy opened a new pull request, #1871:
URL: https://github.com/apache/solr/pull/1871
https://issues.apache.org/jira/browse/SOLR-16954
--
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 merged pull request #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy merged PR #1871:
URL: https://github.com/apache/solr/pull/1871
--
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 diff in pull request #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on code in PR #1871:
URL: https://github.com/apache/solr/pull/1871#discussion_r1315168456
##########
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
+ * want to check circuit breakers.
+ *
+ * @return true if circuit breakers are tripped, false otherwise.
+ */
+ protected boolean checkCircuitBreakers(SolrQueryRequest req, SolrQueryResponse rsp) {
+ CircuitBreakerRegistry circuitBreakerRegistry = req.getCore().getCircuitBreakerRegistry();
+ if (circuitBreakerRegistry.isEnabled(RequestType.UPDATE)) {
+ List<CircuitBreaker> trippedCircuitBreakers =
+ circuitBreakerRegistry.checkTripped(RequestType.UPDATE);
+ if (trippedCircuitBreakers != null) {
+ String errorMessage = CircuitBreakerRegistry.toErrorMessage(trippedCircuitBreakers);
+ rsp.add(STATUS, FAILURE);
+ rsp.setException(
+ new SolrException(
+ SolrException.ErrorCode.SERVICE_UNAVAILABLE,
Review Comment:
I agree this sounds like a more appropritate code. Perhaps it has better support in load balancers/gateways?
The existing CB for query uses 503 code, so we should perhaps not change the code in 9.x.
So we could safely change to 429 in main branch and document it as a breaking change. Or we could change it for the new pluggable CBs, but not for the legacy `CircuitBreakerManager`, providing back compat for existing users/configs. Wdyt?
--
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 #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on PR #1871:
URL: https://github.com/apache/solr/pull/1871#issuecomment-1707853260
Thanks for the attention to detail @cpoerschke. Accepted all your edits. Will merge today.
--
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 #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on PR #1871:
URL: https://github.com/apache/solr/pull/1871#issuecomment-1708028911
Hmm, precommit failed with `run the top-level './gradlew tidy' to format code automatically (see help/formatting.txt for more info).` advisory -- independent of this pull request really, I wonder how practical it might be for the CI to action on that independently i.e. to just run the tidy itself and add a commit to the PR.
--
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 #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on PR #1871:
URL: https://github.com/apache/solr/pull/1871#issuecomment-1698672344
@dsmiley This is the pr for update request circuit breaking. Please have a look.
--
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 diff in pull request #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on code in PR #1871:
URL: https://github.com/apache/solr/pull/1871#discussion_r1315169354
##########
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 search requests above 95% CPU load.
+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>
Review Comment:
Done. Since we switched to a more generic Enum class, we needed to validate supported types separately, so I just documented in ref guide the two supported ones.
--
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 #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on PR #1871:
URL: https://github.com/apache/solr/pull/1871#issuecomment-1705131966
@dsmiley Preparing to merge this, please weigh in with experience from similar work in your private fork.
--
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 diff in pull request #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
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: 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 diff in pull request #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on code in PR #1871:
URL: https://github.com/apache/solr/pull/1871#discussion_r1308917303
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerRegistry.java:
##########
@@ -64,13 +77,16 @@ public List<CircuitBreaker> checkTripped() {
}
/**
- * Returns true if *any* circuit breaker has triggered, false if none have triggered.
+ * Returns true if *any* circuit breaker has triggered for a given request type, false if none
+ * have triggered.
*
- * <p>NOTE: This method short circuits the checking of circuit breakers -- the method will return
- * as soon as it finds a circuit breaker that has triggered.
+ * @param requestType Request type to check for.
+ * <p>NOTE: This method short circuits the checking of circuit breakers -- the method will
+ * return as soon as it finds a circuit breaker that is enabled and has triggered.
Review Comment:
```suggestion
* return as soon as it finds a circuit breaker that has triggered for the given request type.
```
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/RequestType.java:
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.util.circuitbreaker;
+
+/*
+ * Request types that can be registered for a CB
Review Comment:
```suggestion
* Request types that can be registered for a circuit breaker
```
or
```suggestion
* Request types that can be registered for a {@link CircuitBreaker}
```
##########
solr/core/src/java/org/apache/solr/handler/ContentStreamHandlerBase.java:
##########
@@ -63,6 +70,17 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
SolrException.ErrorCode.SERVER_ERROR,
"Updates are temporarily paused for core: " + req.getCore().getName());
}
+ List<CircuitBreaker> trippedCircuitBreakers =
+ req.getCore().getCircuitBreakerRegistry().checkTripped(RequestType.UPDATE);
Review Comment:
Noting that in `SearchHandler` there is a `if (circuitBreakerRegistry.isEnabled())` guard -- wondering if it might be helpful here too?
##########
solr/core/src/java/org/apache/solr/handler/ContentStreamHandlerBase.java:
##########
@@ -63,6 +70,17 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
SolrException.ErrorCode.SERVER_ERROR,
"Updates are temporarily paused for core: " + req.getCore().getName());
}
+ List<CircuitBreaker> trippedCircuitBreakers =
+ req.getCore().getCircuitBreakerRegistry().checkTripped(RequestType.UPDATE);
+ if (trippedCircuitBreakers != null) {
+ String errorMessage = CircuitBreakerRegistry.toErrorMessage(trippedCircuitBreakers);
+ rsp.add(STATUS, FAILURE);
+ rsp.setException(
+ new SolrException(
+ SolrException.ErrorCode.SERVICE_UNAVAILABLE,
+ "Circuit Breakers tripped " + errorMessage));
+ return;
Review Comment:
I think a `solrCoreState.deregisterInFlightUpdate();` call is needed before returning, or for the circuit breaker to be checked first before registering the update as in-flight.
--
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 diff in pull request #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on code in PR #1871:
URL: https://github.com/apache/solr/pull/1871#discussion_r1315493607
##########
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
+ * want to check circuit breakers.
+ *
+ * @return true if circuit breakers are tripped, false otherwise.
+ */
+ protected boolean checkCircuitBreakers(SolrQueryRequest req, SolrQueryResponse rsp) {
+ CircuitBreakerRegistry circuitBreakerRegistry = req.getCore().getCircuitBreakerRegistry();
+ if (circuitBreakerRegistry.isEnabled(RequestType.UPDATE)) {
+ List<CircuitBreaker> trippedCircuitBreakers =
+ circuitBreakerRegistry.checkTripped(RequestType.UPDATE);
+ if (trippedCircuitBreakers != null) {
+ String errorMessage = CircuitBreakerRegistry.toErrorMessage(trippedCircuitBreakers);
+ rsp.add(STATUS, FAILURE);
+ rsp.setException(
+ new SolrException(
+ SolrException.ErrorCode.SERVICE_UNAVAILABLE,
Review Comment:
So I implemented code 429 for the new pluggable CBs, but keeping 503 for `CircuitBreakerManager` for back compat.
--
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 diff in pull request #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on code in PR #1871:
URL: https://github.com/apache/solr/pull/1871#discussion_r1313172009
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreaker.java:
##########
@@ -52,4 +58,22 @@ 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 all request types.
Review Comment:
https://github.com/apache/solr/pull/1871/commits/8d30678dc6431974ae1d742f9919b0b8f9134c5c to replace the backtick usage.
--
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 diff in pull request #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on code in PR #1871:
URL: https://github.com/apache/solr/pull/1871#discussion_r1315162891
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/RequestType.java:
##########
@@ -0,0 +1,23 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.util.circuitbreaker;
+
+/** Request types that can be registered for a {@link CircuitBreaker} */
Review Comment:
Good point. We already have `SolrRequest.SolrRequestType` which fits nicely. Will switch to using it.
--
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 diff in pull request #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on code in PR #1871:
URL: https://github.com/apache/solr/pull/1871#discussion_r1310115919
##########
solr/core/src/java/org/apache/solr/handler/ContentStreamHandlerBase.java:
##########
@@ -49,6 +56,21 @@ public void init(NamedList<?> args) {
@Override
public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+ CircuitBreakerRegistry circuitBreakerRegistry = req.getCore().getCircuitBreakerRegistry();
+ if (circuitBreakerRegistry.isEnabled(RequestType.UPDATE)) {
+ List<CircuitBreaker> trippedCircuitBreakers =
+ circuitBreakerRegistry.checkTripped(RequestType.UPDATE);
+ if (trippedCircuitBreakers != null) {
+ String errorMessage = CircuitBreakerRegistry.toErrorMessage(trippedCircuitBreakers);
+ rsp.add(STATUS, FAILURE);
+ rsp.setException(
+ new SolrException(
+ SolrException.ErrorCode.SERVICE_UNAVAILABLE,
+ "Circuit Breakers tripped " + errorMessage));
+ return;
+ }
+ }
+
Review Comment:
subjective/possibly niche use case: having a `protected boolean checkCircuitBreakers(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception` method called as `if (checkCircuitBreakers(req, rsp) return;` could allow customisation e.g. overriding as no-op for one update handler but not another or to log-and-ignore the tripped circuit breakers if the `req` contained some special fields or something.
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerRegistry.java:
##########
@@ -97,6 +94,10 @@ public static String toErrorMessage(List<CircuitBreaker> circuitBreakerList) {
}
public boolean isEnabled() {
- return !circuitBreakerList.isEmpty();
+ return !circuitBreakerMap.isEmpty();
+ }
Review Comment:
Could this be removed now then actually?
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreaker.java:
##########
@@ -52,4 +58,22 @@ 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 all request types.
Review Comment:
```suggestion
* circuit breaker will be checked for the `SEARCH` request type only.
```
--
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 diff in pull request #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on code in PR #1871:
URL: https://github.com/apache/solr/pull/1871#discussion_r1309302038
##########
solr/core/src/java/org/apache/solr/handler/ContentStreamHandlerBase.java:
##########
@@ -63,6 +70,17 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
SolrException.ErrorCode.SERVER_ERROR,
"Updates are temporarily paused for core: " + req.getCore().getName());
}
+ List<CircuitBreaker> trippedCircuitBreakers =
+ req.getCore().getCircuitBreakerRegistry().checkTripped(RequestType.UPDATE);
+ if (trippedCircuitBreakers != null) {
+ String errorMessage = CircuitBreakerRegistry.toErrorMessage(trippedCircuitBreakers);
+ rsp.add(STATUS, FAILURE);
+ rsp.setException(
+ new SolrException(
+ SolrException.ErrorCode.SERVICE_UNAVAILABLE,
+ "Circuit Breakers tripped " + errorMessage));
+ return;
Review Comment:
Placed it on top before registering.
--
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 diff in pull request #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on code in PR #1871:
URL: https://github.com/apache/solr/pull/1871#discussion_r1315609098
##########
solr/core/src/test-files/solr/collection1/conf/solrconfig-pluggable-circuitbreaker.xml:
##########
@@ -80,6 +80,18 @@
<circuitBreaker class="solr.MemoryCircuitBreaker">
<double name="threshold">75</double>
+ <arr name="requestTypes">
+ <str>update</str>
+ </arr>
+ </circuitBreaker>
+
+ <circuitBreaker class="solr.MemoryCircuitBreaker">
+ <double name="threshold">80</double>
+<!-- Default is search
Review Comment:
```suggestion
<!-- Default is query
```
##########
solr/core/src/test-files/solr/collection1/conf/solrconfig-pluggable-circuitbreaker.xml:
##########
@@ -80,6 +80,18 @@
<circuitBreaker class="solr.MemoryCircuitBreaker">
<double name="threshold">75</double>
+ <arr name="requestTypes">
+ <str>update</str>
+ </arr>
+ </circuitBreaker>
+
+ <circuitBreaker class="solr.MemoryCircuitBreaker">
+ <double name="threshold">80</double>
+<!-- Default is search
+ <arr name="requestTypes">
+ <str>search</str>
Review Comment:
```suggestion
<str>query</str>
```
--
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] psalagnac commented on a diff in pull request #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
Posted by "psalagnac (via GitHub)" <gi...@apache.org>.
psalagnac commented on code in PR #1871:
URL: https://github.com/apache/solr/pull/1871#discussion_r1315124617
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/RequestType.java:
##########
@@ -0,0 +1,23 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.util.circuitbreaker;
+
+/** Request types that can be registered for a {@link CircuitBreaker} */
Review Comment:
Adding this classification for request types is great and probably could have plenty of usages, not only for circuit breaker.
I suggest this is moved to a more generic package.
##########
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 search requests above 95% CPU load.
+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>
Review Comment:
I would add in the doc that possible values for `requestsTypes `are defined in enum `RequestType`.
##########
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
+ * want to check circuit breakers.
+ *
+ * @return true if circuit breakers are tripped, false otherwise.
+ */
+ protected boolean checkCircuitBreakers(SolrQueryRequest req, SolrQueryResponse rsp) {
+ CircuitBreakerRegistry circuitBreakerRegistry = req.getCore().getCircuitBreakerRegistry();
+ if (circuitBreakerRegistry.isEnabled(RequestType.UPDATE)) {
+ List<CircuitBreaker> trippedCircuitBreakers =
+ circuitBreakerRegistry.checkTripped(RequestType.UPDATE);
+ if (trippedCircuitBreakers != null) {
+ String errorMessage = CircuitBreakerRegistry.toErrorMessage(trippedCircuitBreakers);
+ rsp.add(STATUS, FAILURE);
+ rsp.setException(
+ new SolrException(
+ SolrException.ErrorCode.SERVICE_UNAVAILABLE,
Review Comment:
Have you considered using a different error code for circuit breakers?
HTTP `429 Too Many Requests` could be appropriate here. Mostly, if we want the Solr caller to slow down, it is required to somehow expose that we're not just facing a generic failure but some self protection mechanisms.
--
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 diff in pull request #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on code in PR #1871:
URL: https://github.com/apache/solr/pull/1871#discussion_r1313122139
##########
solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java:
##########
@@ -355,18 +356,18 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
final RTimerTree timer = rb.isDebug() ? req.getRequestTimer() : null;
final CircuitBreakerRegistry circuitBreakerRegistry = req.getCore().getCircuitBreakerRegistry();
- if (circuitBreakerRegistry.isEnabled()) {
+ if (circuitBreakerRegistry.isEnabled(RequestType.SEARCH)) {
Review Comment:
Merge conflict with #1882 here, sorry about that. Is it okay if I go ahead and resolve it?
--
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] psalagnac commented on a diff in pull request #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
Posted by "psalagnac (via GitHub)" <gi...@apache.org>.
psalagnac commented on code in PR #1871:
URL: https://github.com/apache/solr/pull/1871#discussion_r1315601759
##########
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
+ * want to check circuit breakers.
+ *
+ * @return true if circuit breakers are tripped, false otherwise.
+ */
+ protected boolean checkCircuitBreakers(SolrQueryRequest req, SolrQueryResponse rsp) {
+ CircuitBreakerRegistry circuitBreakerRegistry = req.getCore().getCircuitBreakerRegistry();
+ if (circuitBreakerRegistry.isEnabled(RequestType.UPDATE)) {
+ List<CircuitBreaker> trippedCircuitBreakers =
+ circuitBreakerRegistry.checkTripped(RequestType.UPDATE);
+ if (trippedCircuitBreakers != null) {
+ String errorMessage = CircuitBreakerRegistry.toErrorMessage(trippedCircuitBreakers);
+ rsp.add(STATUS, FAILURE);
+ rsp.setException(
+ new SolrException(
+ SolrException.ErrorCode.SERVICE_UNAVAILABLE,
Review Comment:
Good point about compatibility of legacy `CircuitBreakerManager`. Thanks!
--
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 diff in pull request #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on code in PR #1871:
URL: https://github.com/apache/solr/pull/1871#discussion_r1315605803
##########
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
+ * want to check circuit breakers.
+ *
+ * @return true if circuit breakers are tripped, false otherwise.
+ */
+ protected boolean checkCircuitBreakers(SolrQueryRequest req, SolrQueryResponse rsp) {
+ CircuitBreakerRegistry circuitBreakerRegistry = req.getCore().getCircuitBreakerRegistry();
+ if (circuitBreakerRegistry.isEnabled(RequestType.UPDATE)) {
+ List<CircuitBreaker> trippedCircuitBreakers =
+ circuitBreakerRegistry.checkTripped(RequestType.UPDATE);
+ if (trippedCircuitBreakers != null) {
+ String errorMessage = CircuitBreakerRegistry.toErrorMessage(trippedCircuitBreakers);
+ rsp.add(STATUS, FAILURE);
+ rsp.setException(
+ new SolrException(
+ SolrException.ErrorCode.SERVICE_UNAVAILABLE,
Review Comment:
Yep, we can remove `CircuitBreakerManager` along with the 503 code handling logic in main branch (in another PR). There is a deprecation notice and upgrade note as part of #96 that alerts people of it going away.
--
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 #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on PR #1871:
URL: https://github.com/apache/solr/pull/1871#issuecomment-1706567335
Will merge this tomorrow Wednesday unless more feedback.
--
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 diff in pull request #1871: SOLR-16954 Make Circuit Breakers available for Update Requests
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on code in PR #1871:
URL: https://github.com/apache/solr/pull/1871#discussion_r1316882036
##########
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:
I think we can defer this, as the user will have a workaround, to switch to the pluggable CBs. We can document this fact in refguide (other issue).
--
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