Skip to content

Commit

Permalink
Remove CollectorManager#forSequentialExecution
Browse files Browse the repository at this point in the history
We have recently (see apache#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.
  • Loading branch information
javanna committed Sep 14, 2024
1 parent aa86a81 commit 141fb54
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 38 deletions.
3 changes: 0 additions & 3 deletions lucene/CHANGES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
---------------------

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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;
}
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
}
});
}

/**
Expand Down

0 comments on commit 141fb54

Please sign in to comment.