cpoerschke commented on code in PR #2059:
URL: https://github.com/apache/solr/pull/2059#discussion_r1381636810
##########
solr/core/src/test/org/apache/solr/util/TestCircuitBreakers.java:
##########
@@ -66,26 +76,39 @@ protected static void indexDocs() {
public void tearDown() throws Exception {
super.tearDown();
dummyMemBreaker.close();
- dummyCBManager.close();
}
@After
public void after() {
removeAllExistingCircuitBreakers();
}
- public void testCBAlwaysTrips() {
+ public void testCBAlwaysTripsWithCorrectCode() {
removeAllExistingCircuitBreakers();
CircuitBreaker circuitBreaker = new MockCircuitBreaker(true);
h.getCore().getCircuitBreakerRegistry().register(circuitBreaker);
- expectThrows(
- SolrException.class,
- () -> {
- h.query(req("name:\"john smith\""));
- });
+ List.of(
+ SolrException.ErrorCode.TOO_MANY_REQUESTS.code,
+ SolrException.ErrorCode.SERVICE_UNAVAILABLE.code,
+ SolrException.ErrorCode.BAD_REQUEST.code)
+ .forEach(
+ code -> {
+ if (code != SolrException.ErrorCode.TOO_MANY_REQUESTS.code) {
+ System.setProperty(
+ CircuitBreaker.SYSPROP_SOLR_CIRCUITBREAKER_ERRORCODE,
String.valueOf(code));
+ }
+ SolrException ex =
+ expectThrows(
+ SolrException.class,
+ () -> {
+ h.query(req("name:\"john smith\""));
+ });
+ assertEquals(code == -1 ? 429 : code, ex.code());
Review Comment:
question: when could the code be `-1` based on the list above?
```suggestion
assertEquals(code == -1 ?
SolrException.ErrorCode.TOO_MANY_REQUESTS.code : code, ex.code());
```
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreaker.java:
##########
@@ -62,6 +64,16 @@ public CircuitBreaker() {}
/** Get error message when the circuit breaker triggers */
public abstract String getErrorMessage();
+ /**
+ * Get http error code, defaults to {@link
SolrException.ErrorCode#TOO_MANY_REQUESTS} but can be
+ * overridden with system property {@link
#SYSPROP_SOLR_CIRCUITBREAKER_ERRORCODE}
+ */
+ public static SolrException.ErrorCode getErrorCode() {
+ return SolrException.ErrorCode.getErrorCode(
+ Integer.getInteger(
+ SYSPROP_SOLR_CIRCUITBREAKER_ERRORCODE,
SolrException.ErrorCode.TOO_MANY_REQUESTS.code));
Review Comment:
edge case: wondering about noisily failing if the property is specified but
invalid e.g. `-Dsolr.circuitbreaker.errorcode=SERVICE_UNAVAILABLE` or some such
misunderstanding.
##########
solr/core/src/test/org/apache/solr/util/TestCircuitBreakers.java:
##########
@@ -66,26 +76,39 @@ protected static void indexDocs() {
public void tearDown() throws Exception {
super.tearDown();
dummyMemBreaker.close();
- dummyCBManager.close();
}
@After
public void after() {
removeAllExistingCircuitBreakers();
}
- public void testCBAlwaysTrips() {
+ public void testCBAlwaysTripsWithCorrectCode() {
removeAllExistingCircuitBreakers();
CircuitBreaker circuitBreaker = new MockCircuitBreaker(true);
h.getCore().getCircuitBreakerRegistry().register(circuitBreaker);
- expectThrows(
- SolrException.class,
- () -> {
- h.query(req("name:\"john smith\""));
- });
+ List.of(
+ SolrException.ErrorCode.TOO_MANY_REQUESTS.code,
+ SolrException.ErrorCode.SERVICE_UNAVAILABLE.code,
+ SolrException.ErrorCode.BAD_REQUEST.code)
+ .forEach(
+ code -> {
+ if (code != SolrException.ErrorCode.TOO_MANY_REQUESTS.code) {
Review Comment:
setting to "too many requests" should also work, right? could we maybe use
`null` to indicate do-not-set?
```suggestion
if (code != null) {
```
##########
solr/core/src/test/org/apache/solr/util/TestCircuitBreakers.java:
##########
@@ -66,26 +76,39 @@ protected static void indexDocs() {
public void tearDown() throws Exception {
super.tearDown();
dummyMemBreaker.close();
- dummyCBManager.close();
}
@After
public void after() {
removeAllExistingCircuitBreakers();
}
- public void testCBAlwaysTrips() {
+ public void testCBAlwaysTripsWithCorrectCode() {
removeAllExistingCircuitBreakers();
CircuitBreaker circuitBreaker = new MockCircuitBreaker(true);
h.getCore().getCircuitBreakerRegistry().register(circuitBreaker);
- expectThrows(
- SolrException.class,
- () -> {
- h.query(req("name:\"john smith\""));
- });
+ List.of(
+ SolrException.ErrorCode.TOO_MANY_REQUESTS.code,
Review Comment:
```suggestion
null,
SolrException.ErrorCode.TOO_MANY_REQUESTS.code,
```
--
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]
