madrob commented on a change in pull request #1606:
URL: https://github.com/apache/lucene-solr/pull/1606#discussion_r445188767
##########
File path:
solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java
##########
@@ -44,25 +48,20 @@ public boolean isCircuitBreakerGauntletTripped() {
return false;
}
- allowedMemory = getCurrentMemoryThreshold();
+ allowedMemory.set(getCurrentMemoryThreshold());
- seenMemory = calculateLiveMemoryUsage();
+ seenMemory.set(calculateLiveMemoryUsage());
- return (seenMemory >= allowedMemory);
+ return (seenMemory.get() >= allowedMemory.get());
}
@Override
public String printDebugInfo() {
- return "seen memory=" + seenMemory + " allowed memory=" + allowedMemory;
+ return "seenMemory=" + seenMemory + " allowedMemory=" + allowedMemory;
Review comment:
Need `.get()` here.
##########
File path:
solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java
##########
@@ -289,6 +295,23 @@ public void handleRequestBody(SolrQueryRequest req,
SolrQueryResponse rsp) throw
rb.requestInfo.setResponseBuilder(rb);
}
+ //TODO: Should this be for indexing requests as well?
+ CircuitBreakerManager circuitBreakerManager =
req.getCore().getCircuitBreakerManager();
+ Map<CircuitBreakerType, CircuitBreaker> trippedCircuitBreakers =
circuitBreakerManager.checkAllCircuitBreakers();
+
+ if (trippedCircuitBreakers != null) {
+ final RTimerTree timer = rb.isDebug() ? req.getRequestTimer() : null;
+
+ if (timer != null) {
+ RTimerTree subt = timer.sub("circuitbreaker");
+ rb.setTimer(subt.sub("circuitbreaker"));
+ }
+ String errorMessage =
CircuitBreakerManager.constructFinalErrorMessageString(trippedCircuitBreakers);
+ rsp.add(STATUS, FAILURE);
+ rsp.setException(new
SolrException(SolrException.ErrorCode.SERVICE_UNAVAILABLE, "Circuit Breakers
tripped " + errorMessage));
Review comment:
I'm not sure this is the right error code. A 5xx code usually indicates
a server error - and I'm not sure how we effectively convey to clients that
this error is something that is ok to retry. They might log the message, but
retry logic will typically look at the code returned as a first branch in the
decision tree. Need to think about this and maybe look at some examples.
##########
File path:
solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java
##########
@@ -289,6 +295,23 @@ public void handleRequestBody(SolrQueryRequest req,
SolrQueryResponse rsp) throw
rb.requestInfo.setResponseBuilder(rb);
}
+ //TODO: Should this be for indexing requests as well?
+ CircuitBreakerManager circuitBreakerManager =
req.getCore().getCircuitBreakerManager();
+ Map<CircuitBreakerType, CircuitBreaker> trippedCircuitBreakers =
circuitBreakerManager.checkAllCircuitBreakers();
+
+ if (trippedCircuitBreakers != null) {
+ final RTimerTree timer = rb.isDebug() ? req.getRequestTimer() : null;
+
+ if (timer != null) {
+ RTimerTree subt = timer.sub("circuitbreaker");
Review comment:
We never stop this subtimer.
##########
File path:
solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java
##########
@@ -289,6 +295,23 @@ public void handleRequestBody(SolrQueryRequest req,
SolrQueryResponse rsp) throw
rb.requestInfo.setResponseBuilder(rb);
}
+ //TODO: Should this be for indexing requests as well?
+ CircuitBreakerManager circuitBreakerManager =
req.getCore().getCircuitBreakerManager();
+ Map<CircuitBreakerType, CircuitBreaker> trippedCircuitBreakers =
circuitBreakerManager.checkAllCircuitBreakers();
+
+ if (trippedCircuitBreakers != null) {
+ final RTimerTree timer = rb.isDebug() ? req.getRequestTimer() : null;
Review comment:
I'd combine this with the later call to req.getRequestTimer so that
we're not doing that twice. You're also checking this before we call
rb.setDebug(), so it probably is always false at this point.
We should be timing the circuitBreakerCheck as well (Lines 299-300).
##########
File path: solr/core/src/java/org/apache/solr/core/SolrConfig.java
##########
@@ -224,6 +224,11 @@ private SolrConfig(SolrResourceLoader loader, String name,
boolean isConfigsetTr
queryResultWindowSize = Math.max(1, getInt("query/queryResultWindowSize",
1));
queryResultMaxDocsCached = getInt("query/queryResultMaxDocsCached",
Integer.MAX_VALUE);
enableLazyFieldLoading = getBool("query/enableLazyFieldLoading", false);
+
+ useCircuitBreakers = getBool("query/useCircuitBreakers", false);
Review comment:
I'd like for all of this to be dynamically configurable at some point,
but it doesn't have to be in this PR. Can add it to the future SIP or create a
separate JIRA for it, as you think would be appropriate.
----------------------------------------------------------------
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
