From a4c79c8d3068ce257adcaf019db210e5842ed137 Mon Sep 17 00:00:00 2001
From: Luca Cavanna <javanna@apache.org>
Date: Tue, 17 Sep 2024 09:41:10 +0200
Subject: [PATCH] Remove CollectorManager#forSequentialExecution (#13790)

We have recently (see #13735) introduced this utility method that creates a
collector manager which only works when a searcher does not have an executor
set, otherwise it throws exception once we attempt to create a new collector
for more than one slice.

While we discussed it should be safe to use in some specific scenarios like the
monitor module, we should be careful exposing this utility publicly, because
while we'd like to ease migration from the search(Query, Collector) method, we
may end up making users like even worse, in that it exposes them to failures
whenever an executor is set and there are more than one slice created, which
is hard to follow and does not provide a good user experience.

My proposal is that we use a similar collector manager locally, where safe and
required, but we don't expose it to users. In most places, we should rather
expose collector managers that do support search concurrency, rather than working
around the lack of those.
---
 lucene/CHANGES.txt                            |  3 --
 .../lucene/search/CollectorManager.java       | 34 -------------------
 .../lucene/monitor/CollectingMatcher.java     | 23 ++++++++++++-
 3 files changed, 22 insertions(+), 38 deletions(-)

diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt
index dac9b15853af..fba7e1144c28 100644
--- a/lucene/CHANGES.txt
+++ b/lucene/CHANGES.txt
@@ -311,9 +311,6 @@ API Changes
 * GITHUB#13568, GITHUB#13750: Add DrillSideways#search method that supports any CollectorManagers for drill-sideways dimensions
   or drill-down. (Egor Potemkin)
 
-* GITHUB#13735: Add CollectorManager#forSequentialExecution to make CollectorManager creation more convenient
-  for users of the deprecated IndexSearcher#search(Query, Collector). (Greg Miller)
-
 New Features
 ---------------------
 
diff --git a/lucene/core/src/java/org/apache/lucene/search/CollectorManager.java b/lucene/core/src/java/org/apache/lucene/search/CollectorManager.java
index c33712b13218..3c87a94e36d9 100644
--- a/lucene/core/src/java/org/apache/lucene/search/CollectorManager.java
+++ b/lucene/core/src/java/org/apache/lucene/search/CollectorManager.java
@@ -18,8 +18,6 @@
 
 import java.io.IOException;
 import java.util.Collection;
-import java.util.concurrent.Executor;
-import org.apache.lucene.index.IndexReader;
 import org.apache.lucene.index.LeafReaderContext;
 
 /**
@@ -55,36 +53,4 @@ public interface CollectorManager<C extends Collector, T> {
    * called after collection is finished on all provided collectors.
    */
   T reduce(Collection<C> collectors) throws IOException;
-
-  /**
-   * Wrap a provided {@link Collector} with a thin {@code CollectorManager} wrapper for use with
-   * {@link IndexSearcher#search(Query, CollectorManager)} when doing single-threaded searching. The
-   * wrapping {@code CollectorManager} provides no {@link CollectorManager#reduce(Collection)}
-   * implementation, so the wrapped {@code Collector} needs to do all relevant work while
-   * collecting.
-   *
-   * <p>Note: This is only safe to use when {@code IndexSearcher} is created with no executor (see:
-   * {@link IndexSearcher#IndexSearcher(IndexReader, Executor)}).
-   */
-  static <C extends Collector> CollectorManager<C, ?> forSequentialExecution(C in) {
-    return new CollectorManager<C, Void>() {
-      private boolean newCollectorInvoked;
-
-      @Override
-      public C newCollector() {
-        if (newCollectorInvoked) {
-          throw new IllegalStateException(
-              "newCollector should be invoked at most once. Ensure your IndexSearcher has been created without an Executor.");
-        }
-        newCollectorInvoked = true;
-        return in;
-      }
-
-      @Override
-      public Void reduce(Collection<C> collectors) {
-        assert collectors.size() == 1 : "collectors should contain exactly one collector instance";
-        return null;
-      }
-    };
-  }
 }
diff --git a/lucene/monitor/src/java/org/apache/lucene/monitor/CollectingMatcher.java b/lucene/monitor/src/java/org/apache/lucene/monitor/CollectingMatcher.java
index 6175fec5ca33..c434672fd68d 100644
--- a/lucene/monitor/src/java/org/apache/lucene/monitor/CollectingMatcher.java
+++ b/lucene/monitor/src/java/org/apache/lucene/monitor/CollectingMatcher.java
@@ -18,6 +18,7 @@
 package org.apache.lucene.monitor;
 
 import java.io.IOException;
+import java.util.Collection;
 import java.util.Map;
 import org.apache.lucene.search.CollectorManager;
 import org.apache.lucene.search.IndexSearcher;
@@ -38,9 +39,29 @@ abstract class CollectingMatcher<T extends QueryMatch> extends CandidateMatcher<
   @Override
   public void matchQuery(final String queryId, Query matchQuery, Map<String, String> metadata)
       throws IOException {
+    MatchCollector matchCollector = new MatchCollector(queryId, scoreMode);
     searcher.search(
         matchQuery,
-        CollectorManager.forSequentialExecution(new MatchCollector(queryId, scoreMode)));
+        new CollectorManager<MatchCollector, Void>() {
+          boolean newCollectorInvoked = false;
+
+          @Override
+          public MatchCollector newCollector() {
+            if (newCollectorInvoked) {
+              throw new IllegalStateException(
+                  "newCollector should be invoked at most once. Ensure your IndexSearcher has been created without an Executor.");
+            }
+            newCollectorInvoked = true;
+            return matchCollector;
+          }
+
+          @Override
+          public Void reduce(Collection<MatchCollector> collectors) {
+            assert collectors.size() == 1
+                : "collectors should contain exactly one collector instance";
+            return null;
+          }
+        });
   }
 
   /**