SOLR-17182: Remove solr.useExitableDirectoryReader property and always use EDR. (#3748)

Author: sigram

solr/CHANGES.txt

@@ -189,6 +189,8 @@ Deprecation Removals
 
 * SOLR-17935: Remove deprecated NoOpResponseParser. (Eric Pugh, David Smiley)
 
+* SOLR-17182: Remove `solr.useExitableDirectoryReader` property. (Andrzej Bialecki)
+
 Dependency Upgrades
 ---------------------
 
@@ -266,6 +268,10 @@ Other Changes
 
 * SOLR-17929: Remove obsolete overseer internal work queue. This queue was read-only since Solr 8. (Pierre Salagnac)
 
+* SOLR-17182: Remove `solr.useExitableDirectoryReader` property. Using `ExitableDirectoryReader`
+  apparently has negligible impact on search performance so it's used now for all queries regardless of
+  their query limits params. (Andrzej Bialecki)
+
 ==================  9.10.0 ==================
 New Features
 ---------------------

solr/benchmark/src/java/org/apache/solr/bench/search/ExitableDirectoryReaderSearch.java

@@ -31,7 +31,6 @@
 import org.apache.solr.common.SolrInputDocument;
 import org.apache.solr.common.params.ModifiableSolrParams;
 import org.apache.solr.search.CallerSpecificQueryLimit;
-import org.apache.solr.search.SolrIndexSearcher;
 import org.apache.solr.util.TestInjection;
 import org.openjdk.jmh.annotations.Benchmark;
 import org.openjdk.jmh.annotations.BenchmarkMode;
@@ -109,9 +108,6 @@ public void setupTrial(MiniClusterState.MiniClusterBenchState miniClusterState)
       miniClusterState.dumpCoreInfo();
     }
 
-    @Param({"false", "true"})
-    boolean useEDR;
-
     // this adds significant processing time to the checking of query limits
     // both to verify that it's actually used and to illustrate the impact of limit checking
     @Param({"false", "true"})
@@ -121,7 +117,6 @@ public void setupTrial(MiniClusterState.MiniClusterBenchState miniClusterState)
 
     @Setup(Level.Iteration)
     public void setupQueries(MiniClusterState.MiniClusterBenchState state) throws Exception {
-      System.setProperty(SolrIndexSearcher.EXITABLE_READER_PROPERTY, String.valueOf(useEDR));
       if (verifyEDRInUse) {
         TestInjection.queryTimeout = new CallerSpecificQueryLimit(Set.of(matchExpression));
       }
@@ -139,7 +134,7 @@ public void setupQueries(MiniClusterState.MiniClusterBenchState state) throws Ex
 
     @TearDown(Level.Iteration)
     public void tearDownTrial() throws Exception {
-      if (useEDR && verifyEDRInUse) {
+      if (verifyEDRInUse) {
         CallerSpecificQueryLimit queryLimit = (CallerSpecificQueryLimit) TestInjection.queryTimeout;
         if (queryLimit == null) {
           throw new RuntimeException("Missing setup!");

solr/benchmark/src/java/org/apache/solr/bench/search/JsonFaceting.java

@@ -28,7 +28,6 @@
 import org.apache.solr.client.solrj.request.QueryRequest;
 import org.apache.solr.common.params.ModifiableSolrParams;
 import org.apache.solr.common.util.NamedList;
-import org.apache.solr.search.SolrIndexSearcher;
 import org.openjdk.jmh.annotations.Benchmark;
 import org.openjdk.jmh.annotations.BenchmarkMode;
 import org.openjdk.jmh.annotations.Fork;
@@ -76,9 +75,6 @@ public static class BenchState {
     @Param({"false", "true"})
     boolean useTimeLimit;
 
-    @Param({"false", "true"})
-    boolean useExitableDirectoryReader;
-
     // DV,  // DocValues, collect into ordinal array
     // UIF, // UnInvertedField, collect into ordinal array
     // DVHASH, // DocValues, collect into hash
@@ -109,11 +105,6 @@ public void setup(
 
       System.setProperty("maxMergeAtOnce", "50");
       System.setProperty("segmentsPerTier", "50");
-      if (useExitableDirectoryReader) {
-        System.setProperty(SolrIndexSearcher.EXITABLE_READER_PROPERTY, "true");
-      } else {
-        System.setProperty(SolrIndexSearcher.EXITABLE_READER_PROPERTY, "false");
-      }
 
       miniClusterState.startMiniCluster(nodeCount);
 

solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java

@@ -135,8 +135,6 @@ 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();
@@ -225,10 +223,8 @@ private static DirectoryReader wrapReader(SolrCore core, DirectoryReader reader)
       throws IOException {
     assert reader != null;
     reader = UninvertingReader.wrap(reader, core.getLatestSchema().getUninversionMapper());
-    // see SOLR-16693 and SOLR-17831 for more details
-    if (EnvUtils.getPropertyAsBool(EXITABLE_READER_PROPERTY, Boolean.FALSE)) {
-      reader = ExitableDirectoryReader.wrap(reader, QueryLimitsTimeout.INSTANCE);
-    }
+    // see SOLR-16693, SOLR-17831 and SOLR-17182 for more details
+    reader = ExitableDirectoryReader.wrap(reader, QueryLimitsTimeout.INSTANCE);
     return reader;
   }
 

solr/core/src/test/org/apache/solr/core/ExitableDirectoryReaderTest.java

@@ -20,7 +20,6 @@
 import java.util.Set;
 import org.apache.solr.SolrTestCaseJ4;
 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;
@@ -51,19 +50,7 @@ public void tearDownCore() {
   }
 
   @Test
-  public void testWithExitableDirectoryReader() throws Exception {
-    doTestWithExitableDirectoryReader(true);
-  }
-
-  @Test
-  public void testWithoutExitableDirectoryReader() throws Exception {
-    doTestWithExitableDirectoryReader(false);
-  }
-
-  private void doTestWithExitableDirectoryReader(boolean withExitableDirectoryReader)
-      throws Exception {
-    System.setProperty(
-        SolrIndexSearcher.EXITABLE_READER_PROPERTY, String.valueOf(withExitableDirectoryReader));
+  public void testExitableDirectoryReader() throws Exception {
     initCore("solrconfig-delaying-component.xml", "schema_latest.xml");
     createIndex();
 
@@ -75,14 +62,9 @@ private void doTestWithExitableDirectoryReader(boolean withExitableDirectoryRead
     String q = "name:a*";
     assertJQ(req("q", q), assertionString);
     Map<String, Integer> callCounts = queryLimit.getCallerMatcher().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));
-    }
+    assertTrue(
+        "there should be some calls from ExitableTermsEnum: " + callCounts,
+        callCounts.get(callerExpr) > 0);
 
     // check that the limits are tripped in ExitableDirectoryReader if it's in use
     int maxCount = random().nextInt(10) + 1;
@@ -91,19 +73,10 @@ private void doTestWithExitableDirectoryReader(boolean withExitableDirectoryRead
     TestInjection.queryTimeout = queryLimit;
     // avoid using the cache
     q = "name:b*";
-    if (withExitableDirectoryReader) {
-      assertJQ(req("q", q), failureAssertionString);
-    } else {
-      assertJQ(req("q", q), assertionString);
-    }
+    assertJQ(req("q", q), failureAssertionString);
     callCounts = queryLimit.getCallerMatcher().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));
-    }
+    assertTrue(
+        "there should be at least " + maxCount + " calls from ExitableTermsEnum: " + callCounts,
+        callCounts.get(callerExpr) >= maxCount);
   }
 }