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.
This commit is contained in:
Luca Cavanna 2024-09-17 09:41:10 +02:00 committed by GitHub
parent f4ebed2404
commit a4c79c8d30
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 22 additions and 38 deletions

View File

@ -311,9 +311,6 @@ API Changes
* GITHUB#13568, GITHUB#13750: Add DrillSideways#search method that supports any CollectorManagers for drill-sideways dimensions * GITHUB#13568, GITHUB#13750: Add DrillSideways#search method that supports any CollectorManagers for drill-sideways dimensions
or drill-down. (Egor Potemkin) 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 New Features
--------------------- ---------------------

View File

@ -18,8 +18,6 @@ package org.apache.lucene.search;
import java.io.IOException; import java.io.IOException;
import java.util.Collection; import java.util.Collection;
import java.util.concurrent.Executor;
import org.apache.lucene.index.IndexReader;
import org.apache.lucene.index.LeafReaderContext; 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. * called after collection is finished on all provided collectors.
*/ */
T reduce(Collection<C> collectors) throws IOException; 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;
}
};
}
} }

View File

@ -18,6 +18,7 @@
package org.apache.lucene.monitor; package org.apache.lucene.monitor;
import java.io.IOException; import java.io.IOException;
import java.util.Collection;
import java.util.Map; import java.util.Map;
import org.apache.lucene.search.CollectorManager; import org.apache.lucene.search.CollectorManager;
import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.IndexSearcher;
@ -38,9 +39,29 @@ abstract class CollectingMatcher<T extends QueryMatch> extends CandidateMatcher<
@Override @Override
public void matchQuery(final String queryId, Query matchQuery, Map<String, String> metadata) public void matchQuery(final String queryId, Query matchQuery, Map<String, String> metadata)
throws IOException { throws IOException {
MatchCollector matchCollector = new MatchCollector(queryId, scoreMode);
searcher.search( searcher.search(
matchQuery, 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;
}
});
} }
/** /**