madrob commented on a change in pull request #1606: URL: https://github.com/apache/lucene-solr/pull/1606#discussion_r444955220
########## File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java ########## @@ -0,0 +1,104 @@ +/* + * 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 practise to register new circuit breakers here if you want them checked for every + * request. + */ +public class CircuitBreakerManager { + + private final Map<CircuitBreakerType, CircuitBreaker> circuitBreakerMap = new HashMap<>(); + + // Allows replacing of existing circuit breaker + public void registerCircuitBreaker(CircuitBreakerType circuitBreakerType, CircuitBreaker circuitBreaker) { + assert circuitBreakerType != null && circuitBreaker != null; Review comment: Why is there an assert here? It might be fine to leave it, but I'm curious what the intent is. If you want this check to happen in production, then you probably need to use something like `Objects.assertNotNull()` ########## File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java ########## @@ -0,0 +1,104 @@ +/* + * 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 practise to register new circuit breakers here if you want them checked for every Review comment: nit: s/practise/practice ########## File path: solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java ########## @@ -289,6 +295,19 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw rb.requestInfo.setResponseBuilder(rb); } + //TODO: Should this be for indexing requests as well? Review comment: I think we would want to have two separate controls for this. ########## File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/CircuitBreakerManager.java ########## @@ -0,0 +1,104 @@ +/* + * 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 practise to register new circuit breakers here if you want them checked for every + * request. + */ +public class CircuitBreakerManager { Review comment: A question about the design of this class... We have methods for registering a custom circuit breaker, and some notion of default circuit breakers, but I don't see an easy way to add additional ones. Ideally this would be through the XML or through some kind of schema API or could be very powerful combined with the Package API. I don't have a full idea of what this needs to look like, and this is fine as is to commit now, but I'd really like us to come back to this and develop the idea further. Possibly as a SIP? ########## File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java ########## @@ -0,0 +1,87 @@ +/* + * 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.lang.management.ManagementFactory; +import java.lang.management.MemoryMXBean; + +import org.apache.solr.core.SolrCore; + +public class MemoryCircuitBreaker extends CircuitBreaker { + private static final MemoryMXBean MEMORY_MX_BEAN = ManagementFactory.getMemoryMXBean(); + + private final long currentMaxHeap = MEMORY_MX_BEAN.getHeapMemoryUsage().getMax(); + + // Assumption -- the value of these parameters will be set correctly before invoking printDebugInfo() + private long seenMemory; + private long allowedMemory; + + public MemoryCircuitBreaker(SolrCore solrCore) { + super(solrCore); + } + + // TODO: An optimization can be to trip the circuit breaker for a duration of time + // after the circuit breaker condition is matched. This will optimize for per call + // overhead of calculating the condition parameters but can result in false positives. + @Override + public boolean isCircuitBreakerGauntletTripped() { + if (!isCircuitBreakerEnabled()) { + return false; + } + + allowedMemory = getCurrentMemoryThreshold(); + + seenMemory = calculateLiveMemoryUsage(); + + return (seenMemory >= allowedMemory); + } + + @Override + public String printDebugInfo() { + return "seen memory=" + seenMemory + " allowed memory=" + allowedMemory; + } + + private long getCurrentMemoryThreshold() { + int thresholdValueInPercentage = solrCore.getSolrConfig().memoryCircuitBreakerThreshold; + + if (currentMaxHeap <= 0) { + return Long.MIN_VALUE; Review comment: Does this make sense? It indicates an error condition in the JVM, right? Can move this check to the constructor and throw an Exception since we'd effectively be blocking all queries at that point anyway? ########## File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java ########## @@ -0,0 +1,87 @@ +/* + * 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.lang.management.ManagementFactory; +import java.lang.management.MemoryMXBean; + +import org.apache.solr.core.SolrCore; + +public class MemoryCircuitBreaker extends CircuitBreaker { + private static final MemoryMXBean MEMORY_MX_BEAN = ManagementFactory.getMemoryMXBean(); + + private final long currentMaxHeap = MEMORY_MX_BEAN.getHeapMemoryUsage().getMax(); + + // Assumption -- the value of these parameters will be set correctly before invoking printDebugInfo() + private long seenMemory; + private long allowedMemory; + + public MemoryCircuitBreaker(SolrCore solrCore) { + super(solrCore); + } + + // TODO: An optimization can be to trip the circuit breaker for a duration of time + // after the circuit breaker condition is matched. This will optimize for per call + // overhead of calculating the condition parameters but can result in false positives. + @Override + public boolean isCircuitBreakerGauntletTripped() { + if (!isCircuitBreakerEnabled()) { + return false; + } + + allowedMemory = getCurrentMemoryThreshold(); + + seenMemory = calculateLiveMemoryUsage(); + + return (seenMemory >= allowedMemory); + } + + @Override + public String printDebugInfo() { + return "seen memory=" + seenMemory + " allowed memory=" + allowedMemory; Review comment: s/seen memory/seenMemory - one word for log processing ########## File path: solr/core/src/java/org/apache/solr/util/circuitbreaker/MemoryCircuitBreaker.java ########## @@ -0,0 +1,87 @@ +/* + * 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.lang.management.ManagementFactory; +import java.lang.management.MemoryMXBean; + +import org.apache.solr.core.SolrCore; + +public class MemoryCircuitBreaker extends CircuitBreaker { + private static final MemoryMXBean MEMORY_MX_BEAN = ManagementFactory.getMemoryMXBean(); + + private final long currentMaxHeap = MEMORY_MX_BEAN.getHeapMemoryUsage().getMax(); + + // Assumption -- the value of these parameters will be set correctly before invoking printDebugInfo() + private long seenMemory; + private long allowedMemory; + + public MemoryCircuitBreaker(SolrCore solrCore) { + super(solrCore); + } + + // TODO: An optimization can be to trip the circuit breaker for a duration of time + // after the circuit breaker condition is matched. This will optimize for per call + // overhead of calculating the condition parameters but can result in false positives. + @Override + public boolean isCircuitBreakerGauntletTripped() { + if (!isCircuitBreakerEnabled()) { + return false; + } + + allowedMemory = getCurrentMemoryThreshold(); + + seenMemory = calculateLiveMemoryUsage(); + + return (seenMemory >= allowedMemory); + } + + @Override + public String printDebugInfo() { Review comment: Is this thread safe? If there are multiple queries happening, concurrent with GC, would these values be stale? ########## File path: solr/core/src/test/org/apache/solr/util/TestCircuitBreaker.java ########## @@ -0,0 +1,184 @@ +/* + * 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; + +import java.io.IOException; +import java.util.HashMap; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.common.SolrException; +import org.apache.solr.common.params.CommonParams; +import org.apache.solr.common.util.ExecutorUtil; +import org.apache.solr.common.util.SolrNamedThreadFactory; +import org.apache.solr.core.SolrCore; +import org.apache.solr.search.QueryParsing; +import org.apache.solr.util.circuitbreaker.CircuitBreaker; +import org.apache.solr.util.circuitbreaker.CircuitBreakerType; +import org.apache.solr.util.circuitbreaker.MemoryCircuitBreaker; +import org.junit.AfterClass; +import org.junit.BeforeClass; + +public class TestCircuitBreaker extends SolrTestCaseJ4 { + private final static int NUM_DOCS = 20; + + @BeforeClass + public static void setUpClass() throws Exception { + System.setProperty("filterCache.enabled", "false"); + System.setProperty("queryResultCache.enabled", "false"); + System.setProperty("documentCache.enabled", "true"); + + initCore("solrconfig-memory-circuitbreaker.xml", "schema.xml"); + for (int i = 0 ; i < NUM_DOCS ; i ++) { + assertU(adoc("name", "john smith", "id", "1")); + assertU(adoc("name", "johathon smith", "id", "2")); + assertU(adoc("name", "john percival smith", "id", "3")); + + //commit inside the loop to get multiple segments to make search as realistic as possible + assertU(commit()); + } + } + + @Override + public void tearDown() throws Exception { + super.tearDown(); + } + + @AfterClass + public static void afterClass() { + System.clearProperty("filterCache.enabled"); + System.clearProperty("queryResultCache.enabled"); + System.clearProperty("documentCache.enabled"); + } + + public void testCBAlwaysTrips() throws IOException { + HashMap<String, String> args = new HashMap<String, String>(); + + args.put(QueryParsing.DEFTYPE, CircuitBreaker.NAME); + args.put(CommonParams.FL, "id"); + + CircuitBreaker circuitBreaker = new MockCircuitBreaker(h.getCore()); + + h.getCore().getCircuitBreakerManager().registerCircuitBreaker(CircuitBreakerType.MEMORY, circuitBreaker); + + expectThrows(SolrException.class, () -> { + h.query(req("name:\"john smith\"")); + }); + } + + public void testCBFakeMemoryPressure() throws IOException { + HashMap<String, String> args = new HashMap<String, String>(); + + args.put(QueryParsing.DEFTYPE, CircuitBreaker.NAME); + args.put(CommonParams.FL, "id"); + + CircuitBreaker circuitBreaker = new FakeMemoryPressureCircuitBreaker(h.getCore()); + + h.getCore().getCircuitBreakerManager().registerCircuitBreaker(CircuitBreakerType.MEMORY, circuitBreaker); + + expectThrows(SolrException.class, () -> { + h.query(req("name:\"john smith\"")); + }); + } + + public void testBuildingMemoryPressure() { + ExecutorService executor = ExecutorUtil.newMDCAwareCachedThreadPool( Review comment: Use a try-finally to make sure this executor is shut down. ########## File path: solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java ########## @@ -289,6 +295,19 @@ 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(); Review comment: Do we want to time this and include that in the response (useful for both passed and failed breakers) ---------------------------------------------------------------- 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
