cpoerschke commented on code in PR #1919:
URL: https://github.com/apache/solr/pull/1919#discussion_r1385267122
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/CPUCircuitBreaker.java:
##########
@@ -33,39 +35,47 @@
* We depend on OperatingSystemMXBean which does not allow a configurable
interval of collection of
* data.
*/
-public class CPUCircuitBreaker extends CircuitBreaker {
+public class CPUCircuitBreaker extends CircuitBreaker implements SolrCoreAware
{
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- private boolean enabled = true;
+ private Boolean enabled = null;
private double cpuUsageThreshold;
- private final SolrCore core;
+ private CoreContainer cc;
private static final ThreadLocal<Double> seenCPUUsage =
ThreadLocal.withInitial(() -> 0.0);
private static final ThreadLocal<Double> allowedCPUUsage =
ThreadLocal.withInitial(() -> 0.0);
- public CPUCircuitBreaker(SolrCore core) {
+ public CPUCircuitBreaker() {
super();
- this.core = core;
+ }
+
+ public CPUCircuitBreaker(CoreContainer coreContainer) {
+ super();
+ this.cc = coreContainer;
}
@Override
public void init(NamedList<?> args) {
super.init(args);
- double localSeenCPUUsage = calculateLiveCPUUsage();
-
- if (localSeenCPUUsage < 0) {
- String msg =
- "Initialization failure for CPU circuit breaker. Unable to get
'systemCpuLoad', not supported by the JVM?";
- if (log.isErrorEnabled()) {
- log.error(msg);
- }
- enabled = false;
- }
}
Review Comment:
Can remove `init` then in favour of super method only.
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerRegistry.java:
##########
@@ -111,43 +199,82 @@ public static String toErrorMessage(List<CircuitBreaker>
circuitBreakerList) {
}
public boolean isEnabled(SolrRequestType requestType) {
- return circuitBreakerMap.containsKey(requestType);
+ return circuitBreakerMap.containsKey(requestType)
+ || globalCircuitBreakerMap.containsKey(requestType);
}
@Override
public void close() throws IOException {
synchronized (circuitBreakerMap) {
- final AtomicInteger closeFailedCounter = new AtomicInteger(0);
- circuitBreakerMap
- .values()
- .forEach(
- list ->
- list.forEach(
- it -> {
- try {
- if (log.isDebugEnabled()) {
- log.debug(
- "Closed circuit breaker {} for request type(s)
{}",
- it.getClass().getSimpleName(),
- it.getRequestTypes());
- }
- it.close();
- } catch (IOException e) {
- if (log.isErrorEnabled()) {
- log.error(
- String.format(
- Locale.ROOT,
- "Failed to close circuit breaker %s",
- it.getClass().getSimpleName()),
- e);
- }
- closeFailedCounter.incrementAndGet();
- }
- }));
+ closeCircuitBreakers(
+
circuitBreakerMap.values().stream().flatMap(List::stream).collect(Collectors.toList()));
circuitBreakerMap.clear();
- if (closeFailedCounter.get() > 0) {
- throw new IOException("Failed to close " + closeFailedCounter.get() +
" circuit breakers");
- }
}
}
+
+ private static void closeGlobal() {
+ synchronized (globalCircuitBreakerMap) {
+ closeCircuitBreakers(
+ globalCircuitBreakerMap.values().stream()
+ .flatMap(List::stream)
+ .collect(Collectors.toList()));
+ globalCircuitBreakerMap.clear();
+ }
+ }
+
+ /**
+ * Close a list of circuit breakers, tracing any failures.
+ *
+ * @throws SolrException if any CB fails to close
+ */
+ private static void closeCircuitBreakers(List<CircuitBreaker> breakers) {
+ final AtomicInteger closeFailedCounter = new AtomicInteger(0);
+ breakers.forEach(
+ it -> {
+ try {
+ if (log.isDebugEnabled()) {
+ log.debug(
+ "Closed circuit breaker {} for request type(s) {}",
+ it.getClass().getSimpleName(),
+ it.getRequestTypes());
+ }
+ it.close();
+ } catch (IOException e) {
+ if (log.isErrorEnabled()) {
+ log.error(
+ String.format(
+ Locale.ROOT,
+ "Failed to close circuit breaker %s",
+ it.getClass().getSimpleName()),
Review Comment:
In case of ambiguity e.g. a global search CB and a per-core update CB or so.
```suggestion
"Failed to close circuit breaker %s for request
type(s) {}",
it.getClass().getSimpleName(),
it.getRequestTypes()),
```
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerRegistry.java:
##########
@@ -111,43 +199,82 @@ public static String toErrorMessage(List<CircuitBreaker>
circuitBreakerList) {
}
public boolean isEnabled(SolrRequestType requestType) {
- return circuitBreakerMap.containsKey(requestType);
+ return circuitBreakerMap.containsKey(requestType)
+ || globalCircuitBreakerMap.containsKey(requestType);
Review Comment:
```suggestion
return globalCircuitBreakerMap.containsKey(requestType)
|| circuitBreakerMap.containsKey(requestType);
```
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerRegistry.java:
##########
@@ -111,43 +199,82 @@ public static String toErrorMessage(List<CircuitBreaker>
circuitBreakerList) {
}
public boolean isEnabled(SolrRequestType requestType) {
- return circuitBreakerMap.containsKey(requestType);
+ return circuitBreakerMap.containsKey(requestType)
+ || globalCircuitBreakerMap.containsKey(requestType);
}
@Override
public void close() throws IOException {
synchronized (circuitBreakerMap) {
- final AtomicInteger closeFailedCounter = new AtomicInteger(0);
- circuitBreakerMap
- .values()
- .forEach(
- list ->
- list.forEach(
- it -> {
- try {
- if (log.isDebugEnabled()) {
- log.debug(
- "Closed circuit breaker {} for request type(s)
{}",
- it.getClass().getSimpleName(),
- it.getRequestTypes());
- }
- it.close();
- } catch (IOException e) {
- if (log.isErrorEnabled()) {
- log.error(
- String.format(
- Locale.ROOT,
- "Failed to close circuit breaker %s",
- it.getClass().getSimpleName()),
- e);
- }
- closeFailedCounter.incrementAndGet();
- }
- }));
+ closeCircuitBreakers(
+
circuitBreakerMap.values().stream().flatMap(List::stream).collect(Collectors.toList()));
circuitBreakerMap.clear();
- if (closeFailedCounter.get() > 0) {
- throw new IOException("Failed to close " + closeFailedCounter.get() +
" circuit breakers");
- }
}
}
+
+ private static void closeGlobal() {
+ synchronized (globalCircuitBreakerMap) {
+ closeCircuitBreakers(
+ globalCircuitBreakerMap.values().stream()
+ .flatMap(List::stream)
+ .collect(Collectors.toList()));
+ globalCircuitBreakerMap.clear();
+ }
+ }
+
+ /**
+ * Close a list of circuit breakers, tracing any failures.
+ *
+ * @throws SolrException if any CB fails to close
+ */
+ private static void closeCircuitBreakers(List<CircuitBreaker> breakers) {
+ final AtomicInteger closeFailedCounter = new AtomicInteger(0);
+ breakers.forEach(
+ it -> {
+ try {
+ if (log.isDebugEnabled()) {
+ log.debug(
+ "Closed circuit breaker {} for request type(s) {}",
Review Comment:
```suggestion
"Closing circuit breaker {} for request type(s) {}",
```
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/CPUCircuitBreaker.java:
##########
@@ -33,39 +35,47 @@
* We depend on OperatingSystemMXBean which does not allow a configurable
interval of collection of
* data.
*/
-public class CPUCircuitBreaker extends CircuitBreaker {
+public class CPUCircuitBreaker extends CircuitBreaker implements SolrCoreAware
{
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- private boolean enabled = true;
+ private Boolean enabled = null;
private double cpuUsageThreshold;
- private final SolrCore core;
+ private CoreContainer cc;
private static final ThreadLocal<Double> seenCPUUsage =
ThreadLocal.withInitial(() -> 0.0);
private static final ThreadLocal<Double> allowedCPUUsage =
ThreadLocal.withInitial(() -> 0.0);
- public CPUCircuitBreaker(SolrCore core) {
+ public CPUCircuitBreaker() {
super();
- this.core = core;
+ }
+
+ public CPUCircuitBreaker(CoreContainer coreContainer) {
+ super();
+ this.cc = coreContainer;
}
Review Comment:
Wondering if we need to worry about backwards compatibility here, maybe not
maintaining it but upgrade-note-explaining the signature change perhaps?
`CircuitBreakerRegistry` is tagged as experimental but this class is not i.e.
in principle folks could have sub-classed a custom version of it or so.
##########
solr/core/src/java/org/apache/solr/util/circuitbreaker/CPUCircuitBreaker.java:
##########
@@ -33,39 +35,47 @@
* We depend on OperatingSystemMXBean which does not allow a configurable
interval of collection of
* data.
*/
-public class CPUCircuitBreaker extends CircuitBreaker {
+public class CPUCircuitBreaker extends CircuitBreaker implements SolrCoreAware
{
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- private boolean enabled = true;
+ private Boolean enabled = null;
private double cpuUsageThreshold;
- private final SolrCore core;
+ private CoreContainer cc;
private static final ThreadLocal<Double> seenCPUUsage =
ThreadLocal.withInitial(() -> 0.0);
private static final ThreadLocal<Double> allowedCPUUsage =
ThreadLocal.withInitial(() -> 0.0);
- public CPUCircuitBreaker(SolrCore core) {
+ public CPUCircuitBreaker() {
super();
- this.core = core;
+ }
+
+ public CPUCircuitBreaker(CoreContainer coreContainer) {
+ super();
+ this.cc = coreContainer;
}
@Override
public void init(NamedList<?> args) {
super.init(args);
- double localSeenCPUUsage = calculateLiveCPUUsage();
-
- if (localSeenCPUUsage < 0) {
- String msg =
- "Initialization failure for CPU circuit breaker. Unable to get
'systemCpuLoad', not supported by the JVM?";
- if (log.isErrorEnabled()) {
- log.error(msg);
- }
- enabled = false;
- }
}
@Override
public boolean isTripped() {
+ if (enabled == null) {
+ double localSeenCPUUsage = calculateLiveCPUUsage();
+
+ if (localSeenCPUUsage < 0) {
+ String msg =
+ "Initialization failure for CPU circuit breaker. Unable to get
'systemCpuLoad', not supported by the JVM?";
+ if (log.isErrorEnabled()) {
+ log.error(msg);
+ }
+ enabled = false;
+ } else {
+ enabled = true;
+ }
+ }
Review Comment:
Wondering if this could move into the `inform` method to avoid `Boolean` vs.
`AtomicBoolean` wonderings for `enabled` in case of concurrent `isTripped`
calls.
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
