You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/06/29 12:23:42 UTC
[GitHub] [lucene-solr] ErickErickson commented on a change in pull request #1626: SOLR-14588: Implement Circuit Breakers
ErickErickson commented on a change in pull request #1626:
URL: https://github.com/apache/lucene-solr/pull/1626#discussion_r446684636
##########
File path: solr/CHANGES.txt
##########
@@ -11,6 +11,7 @@ Consult the LUCENE_CHANGES.txt file for additional, low level, changes in this r
New Features
---------------------
* SOLR-14440: Introduce new Certificate Authentication Plugin to load Principal from certificate subject. (Mike Drob)
+* SOLR-14588: Introduce Circuit Breaker Infrastructure and a JVM heap usage memory tracking circuit breaker implementation (Atri Sharma)
Review comment:
Minor nit, should be a new line between these.
##########
File path: solr/contrib/clustering/src/test-files/clustering/solr/collection1/conf/solrconfig.xml
##########
@@ -123,6 +123,17 @@
The purpose is to enable easy caching of user/application level data.
The regenerator argument should be specified as an implementation
of solr.search.CacheRegenerator if autowarming is desired. -->
+
+ <!-- Enable Circuit Breakers
+ -->
+ <useCircuitBreakers>false</useCircuitBreakers>
+
+ <!-- Memory Circuit Breaker Threshold In Percentage
+
+ Post this percentage usage of the heap, incoming queries will be rejected
+ -->
+ <memoryCircuitBreakerThreshold>100</memoryCircuitBreakerThreshold>
Review comment:
A little bit of guidance here would be helpful, maybe "70-80% heap usage is typical". Numbers are pulled out of thin air, just looking for a starting point for a newbie.
##########
File path: solr/core/src/java/org/apache/solr/core/SolrConfig.java
##########
@@ -804,6 +813,14 @@ private void initLibs(SolrResourceLoader loader, boolean isConfigsetTrusted) {
loader.reloadLuceneSPI();
}
+ private void validateMemoryBreakerThreshold() {
+ if (useCircuitBreakers) {
+ if (memoryCircuitBreakerThreshold > 100 || memoryCircuitBreakerThreshold < 0) {
Review comment:
Does it really make sense to allow either of these values? 0 seems like it'd cause everything to break (haven't seen the rest of the code yet, maybe 0 is a special case). 100% seems too late. Do we have any good information about what reasonable upper and lower bounds are? And should we enforce them? Say 50%/90% as a straw-man proposal for discussion...
##########
File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerType.java
##########
@@ -0,0 +1,26 @@
+/*
+ * 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;
+
+/**
+ * Types of circuit breakers
+ */
Review comment:
This seems like an awfully small file, perhaps put this in the abstract CircuitBreaker class? No big deal either way.
##########
File path: solr/core/src/java/org/apache/solr/core/SolrConfig.java
##########
@@ -804,6 +813,14 @@ private void initLibs(SolrResourceLoader loader, boolean isConfigsetTrusted) {
loader.reloadLuceneSPI();
}
+ private void validateMemoryBreakerThreshold() {
+ if (useCircuitBreakers) {
+ if (memoryCircuitBreakerThreshold > 100 || memoryCircuitBreakerThreshold < 0) {
+ throw new IllegalArgumentException("memoryCircuitBreakerThreshold is not a valid percentage");
+ }
Review comment:
Add what valid percentages are here, especially if we decide to enforce as above..
This should also echo the number entered and what the valid limits are, something like:
memoryCircuitBreakerThreshold was set to" + memoryCircuitBreakerThreshold + ". Valid percentages must be between X% and Y%"
##########
File path: solr/contrib/prometheus-exporter/src/test-files/solr/collection1/conf/solrconfig.xml
##########
@@ -83,6 +83,10 @@
<queryResultMaxDocsCached>200</queryResultMaxDocsCached>
+ <useCircuitBreakers>false</useCircuitBreakers>
+
+ <memoryCircuitBreakerThreshold>100</memoryCircuitBreakerThreshold>
Review comment:
Same as above.
##########
File path: solr/core/src/java/org/apache/solr/core/SolrConfig.java
##########
@@ -522,6 +527,10 @@ public SolrRequestParsers getRequestParsers() {
public final int queryResultWindowSize;
public final int queryResultMaxDocsCached;
public final boolean enableLazyFieldLoading;
+
+ // Circuit Breaker Configuration
+ public final boolean useCircuitBreakers;
+ public final int memoryCircuitBreakerThreshold;
Review comment:
I always like to put in default values where the Java defaults wouldn't work, just in case "somehow" this isn't set in future. memoryCircuitBreakerThreshold defaults to 0 in this case, is that OK?
And maybe name this memoryCircuitBreakerThresholdPct?
##########
File path: solr/core/src/test-files/solr/collection1/conf/solrconfig-implicitproperties.xml
##########
@@ -42,6 +42,8 @@
<enableLazyFieldLoading>true</enableLazyFieldLoading>
<queryResultWindowSize>20</queryResultWindowSize>
<queryResultMaxDocsCached>20</queryResultMaxDocsCached>
+ <useCircuitBreakers>false</useCircuitBreakers>
Review comment:
Why is including the new circuitbreaker configs defaulting to false necessary? If they're left out, this should be a no-op, correct?
##########
File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java
##########
@@ -0,0 +1,128 @@
+/*
+ * 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;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.solr.core.SolrCore;
+
+/**
+ * Manages all registered circuit breaker instances. Responsible for a holistic view
+ * of whether a circuit breaker has tripped or not.
+ *
+ * There are two typical ways of using this class's instance:
+ * 1. Check if any circuit breaker has triggered -- and know which circuit breaker has triggered.
+ * 2. Get an instance of a specific circuit breaker and perform checks.
+ *
+ * It is a good practice to register new circuit breakers here if you want them checked for every
+ * request.
+ *
+ * NOTE: The current way of registering new default circuit breakers is minimal and not a long term
+ * solution. There will be a follow up with a SIP for a schema API design.
+ */
+public class CircuitBreakerManager {
+
+ private final Map<CircuitBreakerType, CircuitBreaker> circuitBreakerMap = new HashMap<>();
+
+ // Allows replacing of existing circuit breaker
+ public void registerCircuitBreaker(CircuitBreakerType circuitBreakerType, CircuitBreaker circuitBreaker) {
+ circuitBreakerMap.put(circuitBreakerType, circuitBreaker);
+ }
+
+ public CircuitBreaker getCircuitBreaker(CircuitBreakerType circuitBreakerType) {
+ assert circuitBreakerType != null;
+
+ return circuitBreakerMap.get(circuitBreakerType);
+ }
+
+ /**
+ * Check if any circuit breaker has triggered.
+ * @return CircuitBreakers which have triggered, null otherwise
+ */
+ public Map<CircuitBreakerType, CircuitBreaker> checkAllCircuitBreakersAndReturnTrippedBreakers() {
+ Map<CircuitBreakerType, CircuitBreaker> triggeredCircuitBreakers = null;
Review comment:
Why not just allocate a new HashMap here and avoid the null check below?
Hmmm, I suppose if there aren't any circuit breakers one could return null, NM.
##########
File path: solr/example/example-DIH/solr/db/conf/solrconfig.xml
##########
@@ -502,6 +502,16 @@
-->
<queryResultMaxDocsCached>200</queryResultMaxDocsCached>
+ <!-- Enable Circuit Breakers
+ -->
+ <useCircuitBreakers>false</useCircuitBreakers>
+
+ <!-- Memory Circuit Breaker Threshold In Percentage
+
Review comment:
I don't think there's any reason to include these in all these solrconfig files. I'd be sure they're include as _comments_ at least in server/solr/configsets/_default and sample_techproducts_configs but not elsewhere. Except, of course, the one you use for testing this functionality.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org