diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index ab044e2f409d..6a79180bbf40 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -226,6 +226,15 @@ Other Changes --------------------- (No changes) +================== 9.9.1 ================== +Bug Fixes +--------------------- +* SOLR-17831: ExitableDirectoryReader always initialized with QueryLimits.NONE (Andrzej Białecki) + +Dependency Upgrades +--------------------- +(No changes) + ================== 9.9.0 ================== New Features --------------------- diff --git a/solr/core/src/java/org/apache/solr/search/QueryLimits.java b/solr/core/src/java/org/apache/solr/search/QueryLimits.java index e6e0db5eed94..60d27f061283 100644 --- a/solr/core/src/java/org/apache/solr/search/QueryLimits.java +++ b/solr/core/src/java/org/apache/solr/search/QueryLimits.java @@ -37,12 +37,12 @@ * or other resource limits. Exceeding any specified limit will cause {@link #shouldExit()} to * return true the next time it is checked (it may be checked in either Lucene code or Solr code) */ -public class QueryLimits implements QueryTimeout { +public final class QueryLimits implements QueryTimeout { public static final String UNLIMITED = "This request is unlimited."; private final List limits = new ArrayList<>(3); // timeAllowed, cpu, and memory anticipated - public static QueryLimits NONE = new QueryLimits(); + public static final QueryLimits NONE = new QueryLimits(); private final SolrQueryResponse rsp; private final boolean allowPartialResults; diff --git a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java index 0f0735b66acb..2e1c31691cae 100644 --- a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java +++ b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java @@ -51,6 +51,7 @@ import org.apache.lucene.index.LeafReaderContext; import org.apache.lucene.index.MultiPostingsEnum; import org.apache.lucene.index.PostingsEnum; +import org.apache.lucene.index.QueryTimeout; import org.apache.lucene.index.StoredFieldVisitor; import org.apache.lucene.index.Term; import org.apache.lucene.index.Terms; @@ -137,16 +138,15 @@ public class SolrIndexSearcher extends IndexSearcher implements Closeable, SolrI public static final String STATS_SOURCE = "org.apache.solr.stats_source"; public static final String STATISTICS_KEY = "searcher"; + + public static final String EXITABLE_READER_PROPERTY = "solr.useExitableDirectoryReader"; + // These should *only* be used for debugging or monitoring purposes public static final AtomicLong numOpens = new AtomicLong(); public static final AtomicLong numCloses = new AtomicLong(); private static final Map> NO_GENERIC_CACHES = Collections.emptyMap(); private static final SolrCache[] NO_CACHES = new SolrCache[0]; - // If you find this useful, let us know in dev@solr.apache.org. Likely to be removed eventually. - private static final boolean useExitableDirectoryReader = - Boolean.getBoolean("solr.useExitableDirectoryReader"); - public static final int EXECUTOR_MAX_CPU_THREADS = Runtime.getRuntime().availableProcessors(); private final SolrCore core; @@ -207,14 +207,31 @@ private static DirectoryReader getReader( } } + private static final class QueryLimitsTimeout implements QueryTimeout { + private static final QueryLimitsTimeout INSTANCE = new QueryLimitsTimeout(); + + private QueryLimitsTimeout() {} + + @Override + public boolean shouldExit() { + return QueryLimits.getCurrentLimits().shouldExit(); + } + + @Override + public String toString() { + return QueryLimits.getCurrentLimits().limitStatusMessage(); + } + } + // TODO: wrap elsewhere and return a "map" from the schema that overrides get() ? // this reader supports reopen private static DirectoryReader wrapReader(SolrCore core, DirectoryReader reader) throws IOException { assert reader != null; reader = UninvertingReader.wrap(reader, core.getLatestSchema().getUninversionMapper()); - if (useExitableDirectoryReader) { // SOLR-16693 legacy; may be removed. Probably inefficient. - reader = ExitableDirectoryReader.wrap(reader, QueryLimits.getCurrentLimits()); + // see SOLR-16693 and SOLR-17831 for more details + if (EnvUtils.getPropertyAsBool(EXITABLE_READER_PROPERTY, Boolean.FALSE)) { + reader = ExitableDirectoryReader.wrap(reader, QueryLimitsTimeout.INSTANCE); } return reader; } @@ -775,7 +792,8 @@ public QueryResult search(QueryResult qr, QueryCommand cmd) throws IOException { protected void search(List leaves, Weight weight, Collector collector) throws IOException { QueryLimits queryLimits = QueryLimits.getCurrentLimits(); - if (useExitableDirectoryReader || !queryLimits.isLimitsEnabled()) { + if (EnvUtils.getPropertyAsBool(EXITABLE_READER_PROPERTY, Boolean.FALSE) + || !queryLimits.isLimitsEnabled()) { // no timeout. Pass through to super class super.search(leaves, weight, collector); } else { diff --git a/solr/core/src/test/org/apache/solr/core/ExitableDirectoryReaderTest.java b/solr/core/src/test/org/apache/solr/core/ExitableDirectoryReaderTest.java index f2e4aaa3c8f1..86e4dcc51a0f 100644 --- a/solr/core/src/test/org/apache/solr/core/ExitableDirectoryReaderTest.java +++ b/solr/core/src/test/org/apache/solr/core/ExitableDirectoryReaderTest.java @@ -16,33 +16,18 @@ */ package org.apache.solr.core; -import static org.apache.solr.common.util.Utils.fromJSONString; - import java.util.Map; import org.apache.solr.SolrTestCaseJ4; -import org.apache.solr.metrics.MetricsMap; -import org.apache.solr.metrics.SolrMetricManager; -import org.apache.solr.response.SolrQueryResponse; -import org.junit.BeforeClass; +import org.apache.solr.search.CallerSpecificQueryLimit; +import org.apache.solr.search.SolrIndexSearcher; +import org.apache.solr.util.TestInjection; +import org.junit.After; import org.junit.Test; -/** - * Test that checks that long-running queries are exited by Solr using the SolrQueryTimeoutImpl - * implementation. - */ public class ExitableDirectoryReaderTest extends SolrTestCaseJ4 { - static final int NUM_DOCS = 100; static final String assertionString = "/response/numFound==" + NUM_DOCS; static final String failureAssertionString = "/responseHeader/partialResults==true]"; - static final String longTimeout = "10000"; - static final String sleep = "2"; - - @BeforeClass - public static void beforeClass() throws Exception { - initCore("solrconfig-delaying-component.xml", "schema_latest.xml"); - createIndex(); - } public static void createIndex() { for (int i = 0; i < NUM_DOCS; i++) { @@ -59,167 +44,65 @@ public static void createIndex() { assertU(commit()); } - @Test - public void testPrefixQuery() throws Exception { - String q = "name:a*"; - assertJQ(req("q", q, "timeAllowed", "1", "sleep", sleep), failureAssertionString); - - // do the same query and test for both success, and that the number of documents matched (i.e. - // make sure no partial results were cached) - assertJQ(req("q", q, "timeAllowed", longTimeout), assertionString); - - // this time we should get a query cache hit and hopefully no exception? this may change in the - // future if time checks are put into other places. - - // 2024-4-15: it did change..., and now this fails with 1 or 2 ms and passes with 3ms... I see - // no way this won't be terribly brittle. Maybe TestInjection of some sort to bring this back? - - // assertJQ(req("q", q, "timeAllowed", "2", "sleep", sleep), assertionString); - - // The idea that the request won't time out due to caching is a flawed test methodology, - // It relies on the test running quickly and not stalling. The above test should possibly - // be doing something along the lines of this (but we lack api for it) - // - // SolrCores solrCores = ExitableDirectoryReaderTest.h.getCoreContainer().solrCores; - // List cores = solrCores.getCores(); - // for (SolrCore core : cores) { - // if (<<< find the right core >>> ) { - // ((SolrCache)core.getSearcher().get().<<>> - // } - // } - - // now do the same for the filter cache - // 2024-4-15: this still passes probably because *:* is so fast, but it still worries me - assertJQ(req("q", "*:*", "fq", q, "timeAllowed", "1", "sleep", sleep), failureAssertionString); - - // make sure that the result succeeds this time, and that a bad filter wasn't cached - assertJQ(req("q", "*:*", "fq", q, "timeAllowed", longTimeout), assertionString); - - // test that Long.MAX_VALUE works - assertJQ(req("q", "name:b*", "timeAllowed", Long.toString(Long.MAX_VALUE)), assertionString); - - // negative timeAllowed should disable timeouts. - assertJQ(req("q", "name:c*", "timeAllowed", "-7"), assertionString); + @After + public void tearDownCore() { + deleteCore(); } - // There are lots of assumptions about how/when cache entries should be changed in this method. - // The simple case above shows the root problem without the confusion. testFilterSimpleCase should - // be removed once it is running and this test should be un-ignored and the assumptions verified. - // With all the weirdness, I'm not going to vouch for this test. Feel free to change it. @Test - public void testCacheAssumptions() throws Exception { - String fq = "name:d*"; - SolrCore core = h.getCore(); - MetricsMap filterCacheStats = - (MetricsMap) - ((SolrMetricManager.GaugeWrapper) - core.getCoreMetricManager() - .getRegistry() - .getMetrics() - .get("CACHE.searcher.filterCache")) - .getGauge(); - long fqInserts = (long) filterCacheStats.getValue().get("inserts"); - - MetricsMap queryCacheStats = - (MetricsMap) - ((SolrMetricManager.GaugeWrapper) - core.getCoreMetricManager() - .getRegistry() - .getMetrics() - .get("CACHE.searcher.queryResultCache")) - .getGauge(); - long qrInserts = (long) queryCacheStats.getValue().get("inserts"); - - // This gets 0 docs back. Use 10000 instead of 1 for timeAllowed, and it gets 100 back and the - // for loop below succeeds. - String response = - JQ(req("q", "*:*", "fq", fq, "indent", "true", "timeAllowed", "1", "sleep", sleep)); - Map res = (Map) fromJSONString(response); - Map body = (Map) (res.get("response")); - assertTrue("Should have fewer docs than " + NUM_DOCS, (long) (body.get("numFound")) < NUM_DOCS); - - Map header = (Map) (res.get("responseHeader")); - assertTrue( - "Should have partial results", - (Boolean) (header.get(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY))); - - assertEquals( - "Should NOT have inserted partial results in the cache!", - (long) queryCacheStats.getValue().get("inserts"), - qrInserts); - - assertEquals( - "Should NOT have another insert", - fqInserts, - (long) filterCacheStats.getValue().get("inserts")); - - // At the end of all this, we should have no hits in the queryResultCache. - response = JQ(req("q", "*:*", "fq", fq, "indent", "true", "timeAllowed", longTimeout)); - - // Check that we did insert this one. - assertEquals("Hits should still be 0", (long) filterCacheStats.getValue().get("hits"), 0L); - assertEquals( - "Inserts should be bumped", - (long) filterCacheStats.getValue().get("inserts"), - fqInserts + 1); - - res = (Map) fromJSONString(response); - body = (Map) (res.get("response")); - - assertEquals("Should have exactly " + NUM_DOCS, (long) (body.get("numFound")), NUM_DOCS); - header = (Map) (res.get("responseHeader")); - assertNull( - "Should NOT have partial results", - header.get(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY)); + public void testWithExitableDirectoryReader() throws Exception { + doTestWithExitableDirectoryReader(true); } - // When looking at a problem raised on the user's list I ran across this anomaly with timeAllowed - // This tests for the second query NOT returning partial results, along with some other @Test - public void testQueryResults() throws Exception { - String q = "name:e*"; - SolrCore core = h.getCore(); - MetricsMap queryCacheStats = - (MetricsMap) - ((SolrMetricManager.GaugeWrapper) - core.getCoreMetricManager() - .getRegistry() - .getMetrics() - .get("CACHE.searcher.queryResultCache")) - .getGauge(); - Map nl = queryCacheStats.getValue(); - long inserts = (long) nl.get("inserts"); - - String response = JQ(req("q", q, "indent", "true", "timeAllowed", "1", "sleep", sleep)); - - // The queryResultCache should NOT get an entry here. - nl = queryCacheStats.getValue(); - assertEquals("Should NOT have inserted partial results!", inserts, (long) nl.get("inserts")); - - Map res = (Map) fromJSONString(response); - Map body = (Map) (res.get("response")); - Map header = (Map) (res.get("responseHeader")); - - assertTrue("Should have fewer docs than " + NUM_DOCS, (long) (body.get("numFound")) < NUM_DOCS); - assertTrue( - "Should have partial results", - (Boolean) (header.get(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY))); - - response = JQ(req("q", q, "indent", "true", "timeAllowed", longTimeout)); + public void testWithoutExitableDirectoryReader() throws Exception { + doTestWithExitableDirectoryReader(false); + } - // Check that we did insert this one. - Map nl2 = queryCacheStats.getValue(); - assertEquals("Hits should still be 0", (long) nl.get("hits"), (long) nl2.get("hits")); - assertTrue("Inserts should be bumped", inserts < (long) nl2.get("inserts")); + private void doTestWithExitableDirectoryReader(boolean withExitableDirectoryReader) + throws Exception { + System.setProperty( + SolrIndexSearcher.EXITABLE_READER_PROPERTY, String.valueOf(withExitableDirectoryReader)); + initCore("solrconfig-delaying-component.xml", "schema_latest.xml"); + createIndex(); - res = (Map) fromJSONString(response); - body = (Map) (res.get("response")); - header = (Map) (res.get("responseHeader")); + // create a limit that will not trip but will report calls to shouldExit() + // NOTE: we need to use the inner class name to capture the calls + String callerExpr = "ExitableTermsEnum:-1"; + CallerSpecificQueryLimit queryLimit = new CallerSpecificQueryLimit(callerExpr); + TestInjection.queryTimeout = queryLimit; + String q = "name:a*"; + assertJQ(req("q", q), assertionString); + Map callCounts = queryLimit.getCallCounts(); + if (withExitableDirectoryReader) { + assertTrue( + "there should be some calls from ExitableTermsEnum: " + callCounts, + callCounts.get(callerExpr) > 0); + } else { + assertEquals( + "there should be no calls from ExitableTermsEnum", 0, (int) callCounts.get(callerExpr)); + } - assertEquals("Should have exactly " + NUM_DOCS, NUM_DOCS, (long) (body.get("numFound"))); - Boolean test = (Boolean) (header.get(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY)); - if (test != null) { - assertFalse("Should NOT have partial results", test); + // check that the limits are tripped in ExitableDirectoryReader if it's in use + int maxCount = random().nextInt(10) + 1; + callerExpr = "ExitableTermsEnum:" + maxCount; + queryLimit = new CallerSpecificQueryLimit(callerExpr); + TestInjection.queryTimeout = queryLimit; + // avoid using the cache + q = "name:b*"; + if (withExitableDirectoryReader) { + assertJQ(req("q", q), failureAssertionString); + } else { + assertJQ(req("q", q), assertionString); + } + callCounts = queryLimit.getCallCounts(); + if (withExitableDirectoryReader) { + assertTrue( + "there should be at least " + maxCount + " calls from ExitableTermsEnum: " + callCounts, + callCounts.get(callerExpr) >= maxCount); + } else { + assertEquals( + "there should be no calls from ExitableTermsEnum", 0, (int) callCounts.get(callerExpr)); } } } diff --git a/solr/core/src/test/org/apache/solr/core/TimeAllowedTest.java b/solr/core/src/test/org/apache/solr/core/TimeAllowedTest.java new file mode 100644 index 000000000000..32521802d136 --- /dev/null +++ b/solr/core/src/test/org/apache/solr/core/TimeAllowedTest.java @@ -0,0 +1,225 @@ +/* + * 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.core; + +import static org.apache.solr.common.util.Utils.fromJSONString; + +import java.util.Map; +import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.metrics.MetricsMap; +import org.apache.solr.metrics.SolrMetricManager; +import org.apache.solr.response.SolrQueryResponse; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Test that checks that long-running queries are exited by Solr using the {@link + * org.apache.solr.search.QueryLimits} implementation. + */ +public class TimeAllowedTest extends SolrTestCaseJ4 { + + static final int NUM_DOCS = 100; + static final String assertionString = "/response/numFound==" + NUM_DOCS; + static final String failureAssertionString = "/responseHeader/partialResults==true]"; + static final String longTimeout = "10000"; + static final String sleep = "2"; + + @BeforeClass + public static void beforeClass() throws Exception { + initCore("solrconfig-delaying-component.xml", "schema_latest.xml"); + createIndex(); + } + + public static void createIndex() { + for (int i = 0; i < NUM_DOCS; i++) { + assertU( + adoc( + "id", + Integer.toString(i), + "name", + "a" + i + " b" + i + " c" + i + " d" + i + " e" + i)); + if (random().nextInt(NUM_DOCS) == 0) { + assertU(commit()); // sometimes make multiple segments + } + } + assertU(commit()); + } + + @Test + public void testPrefixQuery() throws Exception { + String q = "name:a*"; + assertJQ(req("q", q, "timeAllowed", "1", "sleep", sleep), failureAssertionString); + + // do the same query and test for both success, and that the number of documents matched (i.e. + // make sure no partial results were cached) + assertJQ(req("q", q, "timeAllowed", longTimeout), assertionString); + + // this time we should get a query cache hit and hopefully no exception? this may change in the + // future if time checks are put into other places. + + // 2024-4-15: it did change..., and now this fails with 1 or 2 ms and passes with 3ms... I see + // no way this won't be terribly brittle. Maybe TestInjection of some sort to bring this back? + + // assertJQ(req("q", q, "timeAllowed", "2", "sleep", sleep), assertionString); + + // The idea that the request won't time out due to caching is a flawed test methodology, + // It relies on the test running quickly and not stalling. The above test should possibly + // be doing something along the lines of this (but we lack api for it) + // + // SolrCores solrCores = ExitableDirectoryReaderTest.h.getCoreContainer().solrCores; + // List cores = solrCores.getCores(); + // for (SolrCore core : cores) { + // if (<<< find the right core >>> ) { + // ((SolrCache)core.getSearcher().get().<<>> + // } + // } + + // now do the same for the filter cache + // 2024-4-15: this still passes probably because *:* is so fast, but it still worries me + assertJQ(req("q", "*:*", "fq", q, "timeAllowed", "1", "sleep", sleep), failureAssertionString); + + // make sure that the result succeeds this time, and that a bad filter wasn't cached + assertJQ(req("q", "*:*", "fq", q, "timeAllowed", longTimeout), assertionString); + + // test that Long.MAX_VALUE works + assertJQ(req("q", "name:b*", "timeAllowed", Long.toString(Long.MAX_VALUE)), assertionString); + + // negative timeAllowed should disable timeouts. + assertJQ(req("q", "name:c*", "timeAllowed", "-7"), assertionString); + } + + // There are lots of assumptions about how/when cache entries should be changed in this method. + // The simple case above shows the root problem without the confusion. testFilterSimpleCase should + // be removed once it is running and this test should be un-ignored and the assumptions verified. + // With all the weirdness, I'm not going to vouch for this test. Feel free to change it. + @Test + public void testCacheAssumptions() throws Exception { + String fq = "name:d*"; + SolrCore core = h.getCore(); + MetricsMap filterCacheStats = + (MetricsMap) + ((SolrMetricManager.GaugeWrapper) + core.getCoreMetricManager() + .getRegistry() + .getMetrics() + .get("CACHE.searcher.filterCache")) + .getGauge(); + long fqInserts = (long) filterCacheStats.getValue().get("inserts"); + + MetricsMap queryCacheStats = + (MetricsMap) + ((SolrMetricManager.GaugeWrapper) + core.getCoreMetricManager() + .getRegistry() + .getMetrics() + .get("CACHE.searcher.queryResultCache")) + .getGauge(); + long qrInserts = (long) queryCacheStats.getValue().get("inserts"); + + // This gets 0 docs back. Use 10000 instead of 1 for timeAllowed, and it gets 100 back and the + // for loop below succeeds. + String response = + JQ(req("q", "*:*", "fq", fq, "indent", "true", "timeAllowed", "1", "sleep", sleep)); + Map res = (Map) fromJSONString(response); + Map body = (Map) (res.get("response")); + assertTrue("Should have fewer docs than " + NUM_DOCS, (long) (body.get("numFound")) < NUM_DOCS); + + Map header = (Map) (res.get("responseHeader")); + assertTrue( + "Should have partial results", + (Boolean) (header.get(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY))); + + assertEquals( + "Should NOT have inserted partial results in the cache!", + (long) queryCacheStats.getValue().get("inserts"), + qrInserts); + + assertEquals( + "Should NOT have another insert", + fqInserts, + (long) filterCacheStats.getValue().get("inserts")); + + // At the end of all this, we should have no hits in the queryResultCache. + response = JQ(req("q", "*:*", "fq", fq, "indent", "true", "timeAllowed", longTimeout)); + + // Check that we did insert this one. + assertEquals("Hits should still be 0", (long) filterCacheStats.getValue().get("hits"), 0L); + assertEquals( + "Inserts should be bumped", + (long) filterCacheStats.getValue().get("inserts"), + fqInserts + 1); + + res = (Map) fromJSONString(response); + body = (Map) (res.get("response")); + + assertEquals("Should have exactly " + NUM_DOCS, (long) (body.get("numFound")), NUM_DOCS); + header = (Map) (res.get("responseHeader")); + assertNull( + "Should NOT have partial results", + header.get(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY)); + } + + // When looking at a problem raised on the user's list I ran across this anomaly with timeAllowed + // This tests for the second query NOT returning partial results, along with some other + @Test + public void testQueryResults() throws Exception { + String q = "name:e*"; + SolrCore core = h.getCore(); + MetricsMap queryCacheStats = + (MetricsMap) + ((SolrMetricManager.GaugeWrapper) + core.getCoreMetricManager() + .getRegistry() + .getMetrics() + .get("CACHE.searcher.queryResultCache")) + .getGauge(); + Map nl = queryCacheStats.getValue(); + long inserts = (long) nl.get("inserts"); + + String response = JQ(req("q", q, "indent", "true", "timeAllowed", "1", "sleep", sleep)); + + // The queryResultCache should NOT get an entry here. + nl = queryCacheStats.getValue(); + assertEquals("Should NOT have inserted partial results!", inserts, (long) nl.get("inserts")); + + Map res = (Map) fromJSONString(response); + Map body = (Map) (res.get("response")); + Map header = (Map) (res.get("responseHeader")); + + assertTrue("Should have fewer docs than " + NUM_DOCS, (long) (body.get("numFound")) < NUM_DOCS); + assertTrue( + "Should have partial results", + (Boolean) (header.get(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY))); + + response = JQ(req("q", q, "indent", "true", "timeAllowed", longTimeout)); + + // Check that we did insert this one. + Map nl2 = queryCacheStats.getValue(); + assertEquals("Hits should still be 0", (long) nl.get("hits"), (long) nl2.get("hits")); + assertTrue("Inserts should be bumped", inserts < (long) nl2.get("inserts")); + + res = (Map) fromJSONString(response); + body = (Map) (res.get("response")); + header = (Map) (res.get("responseHeader")); + + assertEquals("Should have exactly " + NUM_DOCS, NUM_DOCS, (long) (body.get("numFound"))); + Boolean test = (Boolean) (header.get(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY)); + if (test != null) { + assertFalse("Should NOT have partial results", test); + } + } +} diff --git a/solr/core/src/test/org/apache/solr/search/CallerSpecificQueryLimit.java b/solr/core/src/test/org/apache/solr/search/CallerSpecificQueryLimit.java deleted file mode 100644 index 96a6bd5ee937..000000000000 --- a/solr/core/src/test/org/apache/solr/search/CallerSpecificQueryLimit.java +++ /dev/null @@ -1,109 +0,0 @@ -/* - * 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.search; - -import java.lang.invoke.MethodHandles; -import java.util.HashMap; -import java.util.HashSet; -import java.util.Map; -import java.util.Optional; -import java.util.Set; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -/** - * Helper class to simulate query timeouts at specific points in various components that call {@link - * QueryLimits#shouldExit()}. These calling points are identified by the calling class' simple name - * and optionally a method name, e.g. MoreLikeThisComponent or - * ClusteringComponent.finishStage. - */ -public class CallerSpecificQueryLimit implements QueryLimit { - private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); - - final StackWalker stackWalker = - StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE); - final Map> interestingCallers = new HashMap<>(); - public String trippedBy; - - /** - * Signal a timeout in places that match the calling classes (and methods). - * - * @param callerExprs list of expressions in the format of simpleClassName[.methodName] - * . If the list is empty or null then the first call to {@link #shouldExit()} from any - * caller will match. - */ - public CallerSpecificQueryLimit(String... callerExprs) { - if (callerExprs != null && callerExprs.length > 0) { - for (String callerExpr : callerExprs) { - String[] clazzMethod = callerExpr.split("\\."); - if (clazzMethod.length > 2) { - throw new RuntimeException("Invalid callerExpr: " + callerExpr); - } - Set methods = - interestingCallers.computeIfAbsent(clazzMethod[0], c -> new HashSet<>()); - if (clazzMethod.length > 1) { - methods.add(clazzMethod[1]); - } - } - } - } - - @Override - public boolean shouldExit() { - Optional matchingExpr = - stackWalker.walk( - s -> - s.filter( - frame -> { - Class declaring = frame.getDeclaringClass(); - // skip bottom-most frames: myself and QueryLimits - if (declaring == this.getClass() || declaring == QueryLimits.class) { - return false; - } - String method = frame.getMethodName(); - if (interestingCallers.isEmpty()) { - // any caller is an interesting caller - return true; - } - Set methods = interestingCallers.get(declaring.getSimpleName()); - if (methods == null) { - return false; - } - return methods.isEmpty() || methods.contains(method); - }) - .map( - frame -> - (frame.getDeclaringClass().getSimpleName().isBlank() - ? frame.getClassName() - : frame.getDeclaringClass().getSimpleName()) - + "." - + frame.getMethodName()) - .findFirst()); - if (matchingExpr.isPresent()) { - if (log.isInfoEnabled()) { - log.info("++++ Limit tripped by caller: {} ++++", matchingExpr.get()); - } - trippedBy = matchingExpr.get(); - } - return matchingExpr.isPresent(); - } - - @Override - public Object currentValue() { - return "This class just for testing, not a real limit"; - } -} diff --git a/solr/core/src/test/org/apache/solr/search/TestQueryLimits.java b/solr/core/src/test/org/apache/solr/search/TestQueryLimits.java index 7b984bbf05ba..01d7447c181e 100644 --- a/solr/core/src/test/org/apache/solr/search/TestQueryLimits.java +++ b/solr/core/src/test/org/apache/solr/search/TestQueryLimits.java @@ -16,6 +16,7 @@ */ package org.apache.solr.search; +import java.util.Map; import org.apache.lucene.tests.util.TestUtil; import org.apache.solr.client.solrj.SolrClient; import org.apache.solr.client.solrj.request.CollectionAdminRequest; @@ -66,7 +67,7 @@ public void testQueryLimits() throws Exception { "SearchHandler.handleRequestBody", "QueryComponent", "QueryComponent.process", - "FacetComponent.process" + "FacetComponent.process:2" }; for (String matchingExpr : matchingExprTests) { CallerSpecificQueryLimit limit = new CallerSpecificQueryLimit(matchingExpr); @@ -88,13 +89,12 @@ public void testQueryLimits() throws Exception { assertNotNull( "should have partial results for expr " + matchingExpr, rsp.getHeader().get("partialResults")); - if (matchingExpr.contains(".")) { - assertEquals(matchingExpr, limit.trippedBy); - } else { - assertTrue( - "expected result to start with " + matchingExpr + " but was " + limit.trippedBy, - limit.trippedBy.startsWith(matchingExpr)); - } + assertFalse("should have trippedBy info", limit.getTrippedBy().isEmpty()); + assertTrue( + "expected result to start with " + matchingExpr + " but was " + limit.getTrippedBy(), + limit.getTrippedBy().iterator().next().startsWith(matchingExpr)); + Map callCounts = limit.getCallCounts(); + assertTrue("call count should be > 0", callCounts.get(matchingExpr) > 0); } } } diff --git a/solr/test-framework/src/java/org/apache/solr/search/CallerSpecificQueryLimit.java b/solr/test-framework/src/java/org/apache/solr/search/CallerSpecificQueryLimit.java new file mode 100644 index 000000000000..a979e1eff7dd --- /dev/null +++ b/solr/test-framework/src/java/org/apache/solr/search/CallerSpecificQueryLimit.java @@ -0,0 +1,213 @@ +/* + * 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.search; + +import java.lang.invoke.MethodHandles; +import java.util.Arrays; +import java.util.Collection; +import java.util.HashMap; +import java.util.HashSet; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Collectors; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Helper class to simulate query timeouts at specific points in various components that call {@link + * QueryLimits#shouldExit()}. These calling points are identified by the calling class' simple name + * and optionally a method name and the optional maximum count, e.g. MoreLikeThisComponent + * or + * ClusteringComponent.finishStage, ClusteringComponent.finishStage:100. + * + *

NOTE: implementation details cause the expression simpleName to be disabled when + * also any simpleName.anyMethod[:NNN] expression is used for the same class name. + * + *

NOTE 2: when maximum count is a negative number e.g. simpleName.someMethod:-1 + * then only the number of calls to {@link QueryLimits#shouldExit()} for that expression will be + * reported but no limit will be enforced. + */ +public class CallerSpecificQueryLimit implements QueryLimit { + private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); + + private final StackWalker stackWalker = + StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE); + // className -> set of method names + private final Map> interestingCallers = new HashMap<>(); + // expr -> initial count + private final Map maxCounts = new HashMap<>(); + // expr -> current count + private final Map callCounts = new HashMap<>(); + private Set trippedBy = new LinkedHashSet<>(); + + /** + * Signal a timeout in places that match the calling classes (and methods). + * + * @param callerExprs list of expressions in the format of + * simpleClassName[.methodName][:NNN]. If the list is empty or null then the first call + * to {@link #shouldExit()} from any caller will match. + */ + public CallerSpecificQueryLimit(String... callerExprs) { + this(callerExprs != null ? Arrays.asList(callerExprs) : List.of()); + } + + public CallerSpecificQueryLimit(Collection callerExprs) { + for (String callerExpr : callerExprs) { + String[] exprCount = callerExpr.split(":"); + if (exprCount.length > 2) { + throw new RuntimeException("Invalid count in callerExpr: " + callerExpr); + } + String[] clazzMethod = exprCount[0].split("\\."); + if (clazzMethod.length > 2) { + throw new RuntimeException("Invalid method in callerExpr: " + callerExpr); + } + Set methods = + interestingCallers.computeIfAbsent(clazzMethod[0], c -> new HashSet<>()); + if (clazzMethod.length > 1) { + methods.add(clazzMethod[1]); + } + if (exprCount.length > 1) { + try { + int count = Integer.parseInt(exprCount[1]); + maxCounts.put(exprCount[0], count); + callCounts.put(exprCount[0], new AtomicInteger(0)); + } catch (NumberFormatException e) { + throw new RuntimeException("Invalid count in callerExpr: " + callerExpr, e); + } + } + } + } + + /** Returns the set of caller expressions that were tripped. */ + public Set getTrippedBy() { + return trippedBy; + } + + /** Returns a map of tripped caller expressions to their current call counts. */ + public Map getCallCounts() { + return callCounts.entrySet().stream() + .collect( + Collectors.toMap( + e -> + e.getKey() + + (maxCounts.containsKey(e.getKey()) + ? ":" + maxCounts.get(e.getKey()) + : ""), + e -> e.getValue().get())); + } + + @Override + public boolean shouldExit() { + Optional matchingExpr = + stackWalker.walk( + s -> + s.filter( + frame -> { + Class declaring = frame.getDeclaringClass(); + // skip bottom-most frames: myself and QueryLimits + if (declaring == this.getClass() || declaring == QueryLimits.class) { + return false; + } + String method = frame.getMethodName(); + if (interestingCallers.isEmpty()) { + // any caller is an offending caller + String expr = declaring.getSimpleName() + "." + method; + if (log.isInfoEnabled()) { + log.info("++++ Limit tripped by any first caller: {} ++++", expr); + } + trippedBy.add(expr); + callCounts + .computeIfAbsent(expr, k -> new AtomicInteger()) + .incrementAndGet(); + return true; + } + Set methods = interestingCallers.get(declaring.getSimpleName()); + if (methods == null) { + // no class and no methods specified for this class, so skip + return false; + } + // MATCH. Class name was specified, possibly with methods. + // If methods is empty then all methods match, otherwise only the + // specified methods match. + if (methods.isEmpty() || methods.contains(method)) { + String expr = declaring.getSimpleName(); + if (methods.contains(method)) { + expr = expr + "." + method; + } else { + // even though we don't match/enforce at the method level, still + // record the method counts to give better insight into the callers + callCounts + .computeIfAbsent( + declaring.getSimpleName() + "." + method, + k -> new AtomicInteger(0)) + .incrementAndGet(); + } + int currentCount = + callCounts + .computeIfAbsent(expr, k -> new AtomicInteger(0)) + .incrementAndGet(); + // check if we have a max count for this expression + if (maxCounts.containsKey(expr)) { + int maxCount = maxCounts.getOrDefault(expr, 0); + // if max count is negative then just report the call count + if (maxCount < 0) { + maxCount = Integer.MAX_VALUE; + } + if (currentCount > maxCount) { + if (log.isInfoEnabled()) { + log.info( + "++++ Limit tripped by caller: {}, current count: {}, max: {} ++++", + expr, + currentCount, + maxCounts.get(expr)); + } + trippedBy.add(expr + ":" + maxCounts.get(expr)); + return true; + } else { + return false; // max count not reached, not tripped yet + } + } else { + trippedBy.add(expr); + if (log.isInfoEnabled()) { + log.info("++++ Limit tripped by caller: {} ++++", expr); + } + return true; // no max count, so tripped on first call + } + } else { + return false; + } + }) + .map( + frame -> + (frame.getDeclaringClass().getSimpleName().isBlank() + ? frame.getClassName() + : frame.getDeclaringClass().getSimpleName()) + + "." + + frame.getMethodName()) + .findFirst()); + return matchingExpr.isPresent(); + } + + @Override + public Object currentValue() { + return "This class just for testing, not a real limit"; + } +} diff --git a/solr/test-framework/src/test/org/apache/solr/search/CallerSpecificQueryLimitTest.java b/solr/test-framework/src/test/org/apache/solr/search/CallerSpecificQueryLimitTest.java new file mode 100644 index 000000000000..da8f58c73cf4 --- /dev/null +++ b/solr/test-framework/src/test/org/apache/solr/search/CallerSpecificQueryLimitTest.java @@ -0,0 +1,151 @@ +/* + * 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.search; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Set; +import org.apache.solr.SolrTestCaseJ4; +import org.junit.Test; + +public class CallerSpecificQueryLimitTest extends SolrTestCaseJ4 { + + private static class LimitedWorker { + private final CallerSpecificQueryLimit limit; + + static class NestedLimitedWorker { + private final CallerSpecificQueryLimit limit; + + NestedLimitedWorker(CallerSpecificQueryLimit limit) { + this.limit = limit; + } + + public void doWork() { + limit.shouldExit(); + } + } + + private NestedLimitedWorker nestedLimitedWorker; + + LimitedWorker(CallerSpecificQueryLimit limit) { + this.limit = limit; + nestedLimitedWorker = new NestedLimitedWorker(limit); + } + + public void doWork() { + nestedLimitedWorker.doWork(); + limit.shouldExit(); + } + } + + private static class LimitedWorker2 { + private final CallerSpecificQueryLimit limit; + + LimitedWorker2(CallerSpecificQueryLimit limit) { + this.limit = limit; + } + + public void doWork() { + limit.shouldExit(); + } + } + + private static final boolean[] values = {true, false}; + + @Test + public void testLimits() { + for (boolean withNested : values) { + for (boolean withMethod : values) { + for (boolean withCount : values) { + for (boolean shouldTrip : values) { + doTestWithLimit(withNested, withMethod, withCount, shouldTrip); + } + } + } + } + } + + private void doTestWithLimit( + boolean withNested, boolean withMethod, boolean withCount, boolean shouldTrip) { + List nonMatchingCallerExprs = new ArrayList<>(); + List matchingCallCounts = new ArrayList<>(); + String matchingClassName = + withNested + ? LimitedWorker.NestedLimitedWorker.class.getSimpleName() + : LimitedWorker.class.getSimpleName(); + String callerExpr = matchingClassName; + if (withMethod) { + callerExpr += ".doWork"; + } + int count = 1; + if (withCount) { + count = random().nextInt(9) + 1; + if (shouldTrip) { + callerExpr += ":" + count; + } else { + callerExpr += ":-" + count; + } + } else if (!shouldTrip) { + callerExpr += ":-1"; // no limit, should not trip + } + nonMatchingCallerExprs.add(LimitedWorker2.class.getSimpleName()); + nonMatchingCallerExprs.add(LimitedWorker2.class.getSimpleName() + ".doWork"); + if (withNested) { + nonMatchingCallerExprs.add(LimitedWorker.class.getSimpleName()); + } else { + nonMatchingCallerExprs.add(LimitedWorker.NestedLimitedWorker.class.getSimpleName()); + } + matchingCallCounts.add(callerExpr); + if (!withMethod) { + matchingCallCounts.add(matchingClassName + ".doWork"); + } + + CallerSpecificQueryLimit limit = new CallerSpecificQueryLimit(callerExpr); + LimitedWorker limitedWorker = new LimitedWorker(limit); + LimitedWorker2 limitedWorker2 = new LimitedWorker2(limit); + for (int i = 0; i < count * 2; i++) { + limitedWorker2.doWork(); + limitedWorker.doWork(); + } + Set trippedBy = limit.getTrippedBy(); + if (shouldTrip) { + assertFalse("Limit should have been tripped, callerExpr: " + callerExpr, trippedBy.isEmpty()); + for (String nonMatchingCallerExpr : nonMatchingCallerExprs) { + assertFalse( + "Limit should not have been tripped by " + + nonMatchingCallerExpr + + " but was: " + + trippedBy, + trippedBy.contains(nonMatchingCallerExpr)); + } + } else { + assertTrue( + "Limit should not have been tripped, callerExpr: " + + callerExpr + + ", trippedBy: " + + trippedBy, + trippedBy.isEmpty()); + } + Map callCounts = limit.getCallCounts(); + for (String matchingCallCount : matchingCallCounts) { + assertTrue( + "Call count for " + matchingCallCount + " should be > 0, callCounts: " + callCounts, + callCounts.getOrDefault(matchingCallCount, 0) > 0); + } + } +}