javanna commented on code in PR #12523:
URL: https://github.com/apache/lucene/pull/12523#discussion_r1343009944
##########
lucene/core/src/test/org/apache/lucene/search/TestIndexSearcher.java:
##########
@@ -267,7 +266,133 @@ protected LeafSlice[] slices(List<LeafReaderContext>
leaves) {
return slices.toArray(new LeafSlice[0]);
}
};
- searcher.search(new MatchAllDocsQuery(), 10);
- assertEquals(leaves.size(), numExecutions.get());
+ TopDocs topDocs = searcher.search(new MatchAllDocsQuery(), 10);
+ assertTrue(topDocs.totalHits.value > 0);
+ if (leaves.size() <= 1) {
+ assertEquals(0, numExecutions.get());
+ } else {
+ assertEquals(leaves.size(), numExecutions.get());
+ }
+ }
+
+ /**
+ * Tests that when IndexerSearcher runs concurrent searches on multiple
slices if any Exception is
+ * thrown by one of the slice tasks, it will not return until all tasks have
completed.
+ */
+ public void testMultipleSegmentsOnTheExecutorWithException() {
+ List<LeafReaderContext> leaves = reader.leaves();
+ int fixedThreads = leaves.size() == 1 ? 1 : leaves.size() / 2;
+
+ ExecutorService fixedThreadPoolExecutor =
+ Executors.newFixedThreadPool(fixedThreads, new
NamedThreadFactory("concurrent-slices"));
+
+ IndexSearcher searcher =
+ new IndexSearcher(reader, fixedThreadPoolExecutor) {
+ @Override
+ protected LeafSlice[] slices(List<LeafReaderContext> leaves) {
+ ArrayList<LeafSlice> slices = new ArrayList<>();
+ for (LeafReaderContext ctx : leaves) {
+ slices.add(new LeafSlice(Arrays.asList(ctx)));
+ }
+ return slices.toArray(new LeafSlice[0]);
Review Comment:
it would also be fine to call the existing slices method providing 1,1 as
the threshold arguments. That should achieve the same end-goal.
##########
lucene/core/src/test/org/apache/lucene/search/TestIndexSearcher.java:
##########
@@ -267,7 +266,133 @@ protected LeafSlice[] slices(List<LeafReaderContext>
leaves) {
return slices.toArray(new LeafSlice[0]);
}
};
- searcher.search(new MatchAllDocsQuery(), 10);
- assertEquals(leaves.size(), numExecutions.get());
+ TopDocs topDocs = searcher.search(new MatchAllDocsQuery(), 10);
+ assertTrue(topDocs.totalHits.value > 0);
+ if (leaves.size() <= 1) {
+ assertEquals(0, numExecutions.get());
+ } else {
+ assertEquals(leaves.size(), numExecutions.get());
+ }
Review Comment:
I think that this is incorrect, maybe due to a bad merge? That's because we
now unconditionally offload to the executor.
##########
lucene/core/src/test/org/apache/lucene/search/TestIndexSearcher.java:
##########
@@ -267,7 +266,133 @@ protected LeafSlice[] slices(List<LeafReaderContext>
leaves) {
return slices.toArray(new LeafSlice[0]);
}
};
- searcher.search(new MatchAllDocsQuery(), 10);
- assertEquals(leaves.size(), numExecutions.get());
+ TopDocs topDocs = searcher.search(new MatchAllDocsQuery(), 10);
+ assertTrue(topDocs.totalHits.value > 0);
+ if (leaves.size() <= 1) {
+ assertEquals(0, numExecutions.get());
+ } else {
+ assertEquals(leaves.size(), numExecutions.get());
+ }
+ }
+
+ /**
+ * Tests that when IndexerSearcher runs concurrent searches on multiple
slices if any Exception is
+ * thrown by one of the slice tasks, it will not return until all tasks have
completed.
+ */
+ public void testMultipleSegmentsOnTheExecutorWithException() {
+ List<LeafReaderContext> leaves = reader.leaves();
+ int fixedThreads = leaves.size() == 1 ? 1 : leaves.size() / 2;
Review Comment:
how many leaves max may we have ? I am wondering if we may end up creating
too many threads. Would it hurt the test to lower the number of threads?
##########
lucene/core/src/test/org/apache/lucene/search/TestIndexSearcher.java:
##########
@@ -267,7 +266,133 @@ protected LeafSlice[] slices(List<LeafReaderContext>
leaves) {
return slices.toArray(new LeafSlice[0]);
}
};
- searcher.search(new MatchAllDocsQuery(), 10);
- assertEquals(leaves.size(), numExecutions.get());
+ TopDocs topDocs = searcher.search(new MatchAllDocsQuery(), 10);
+ assertTrue(topDocs.totalHits.value > 0);
+ if (leaves.size() <= 1) {
+ assertEquals(0, numExecutions.get());
+ } else {
+ assertEquals(leaves.size(), numExecutions.get());
+ }
+ }
+
+ /**
+ * Tests that when IndexerSearcher runs concurrent searches on multiple
slices if any Exception is
+ * thrown by one of the slice tasks, it will not return until all tasks have
completed.
+ */
+ public void testMultipleSegmentsOnTheExecutorWithException() {
+ List<LeafReaderContext> leaves = reader.leaves();
+ int fixedThreads = leaves.size() == 1 ? 1 : leaves.size() / 2;
+
+ ExecutorService fixedThreadPoolExecutor =
+ Executors.newFixedThreadPool(fixedThreads, new
NamedThreadFactory("concurrent-slices"));
+
+ IndexSearcher searcher =
+ new IndexSearcher(reader, fixedThreadPoolExecutor) {
+ @Override
+ protected LeafSlice[] slices(List<LeafReaderContext> leaves) {
+ ArrayList<LeafSlice> slices = new ArrayList<>();
+ for (LeafReaderContext ctx : leaves) {
+ slices.add(new LeafSlice(Arrays.asList(ctx)));
+ }
+ return slices.toArray(new LeafSlice[0]);
+ }
+ };
+
+ try {
+ AtomicInteger callsToScorer = new AtomicInteger(0);
+ int numExceptions = leaves.size() == 1 ? 1 :
RandomizedTest.randomIntBetween(1, 2);
+ CountDownLatch latch = new CountDownLatch(numExceptions);
+ MatchAllOrThrowExceptionQuery query =
+ new MatchAllOrThrowExceptionQuery(numExceptions, callsToScorer,
latch);
+ RuntimeException exc = expectThrows(RuntimeException.class, () ->
searcher.search(query, 10));
+ // if the TaskExecutor didn't wait for all tasks to finish, this assert
would frequently fail
+ assertEquals(leaves.size(), callsToScorer.get());
+ assertThat(
+ exc.getMessage(),
Matchers.containsString("MatchAllOrThrowExceptionQuery Exception"));
Review Comment:
would it make sense to also check the suppressed exception? That could also
be a separate test if it's easier, without randomized behaviour. Or you could
pre-define the order in which exceptions may be thrown, which is easy to
preserve as you already have an atomic counter. That should make checking the
suppressed exception relatively easy.
##########
lucene/core/src/test/org/apache/lucene/search/TestIndexSearcher.java:
##########
@@ -267,7 +266,133 @@ protected LeafSlice[] slices(List<LeafReaderContext>
leaves) {
return slices.toArray(new LeafSlice[0]);
}
};
- searcher.search(new MatchAllDocsQuery(), 10);
- assertEquals(leaves.size(), numExecutions.get());
+ TopDocs topDocs = searcher.search(new MatchAllDocsQuery(), 10);
+ assertTrue(topDocs.totalHits.value > 0);
+ if (leaves.size() <= 1) {
+ assertEquals(0, numExecutions.get());
+ } else {
+ assertEquals(leaves.size(), numExecutions.get());
+ }
+ }
+
+ /**
+ * Tests that when IndexerSearcher runs concurrent searches on multiple
slices if any Exception is
+ * thrown by one of the slice tasks, it will not return until all tasks have
completed.
+ */
+ public void testMultipleSegmentsOnTheExecutorWithException() {
+ List<LeafReaderContext> leaves = reader.leaves();
+ int fixedThreads = leaves.size() == 1 ? 1 : leaves.size() / 2;
+
+ ExecutorService fixedThreadPoolExecutor =
+ Executors.newFixedThreadPool(fixedThreads, new
NamedThreadFactory("concurrent-slices"));
+
+ IndexSearcher searcher =
+ new IndexSearcher(reader, fixedThreadPoolExecutor) {
+ @Override
+ protected LeafSlice[] slices(List<LeafReaderContext> leaves) {
+ ArrayList<LeafSlice> slices = new ArrayList<>();
+ for (LeafReaderContext ctx : leaves) {
+ slices.add(new LeafSlice(Arrays.asList(ctx)));
+ }
+ return slices.toArray(new LeafSlice[0]);
+ }
+ };
+
+ try {
+ AtomicInteger callsToScorer = new AtomicInteger(0);
+ int numExceptions = leaves.size() == 1 ? 1 :
RandomizedTest.randomIntBetween(1, 2);
+ CountDownLatch latch = new CountDownLatch(numExceptions);
+ MatchAllOrThrowExceptionQuery query =
+ new MatchAllOrThrowExceptionQuery(numExceptions, callsToScorer,
latch);
+ RuntimeException exc = expectThrows(RuntimeException.class, () ->
searcher.search(query, 10));
+ // if the TaskExecutor didn't wait for all tasks to finish, this assert
would frequently fail
+ assertEquals(leaves.size(), callsToScorer.get());
+ assertThat(
+ exc.getMessage(),
Matchers.containsString("MatchAllOrThrowExceptionQuery Exception"));
+ } finally {
+ TestUtil.shutdownExecutorService(fixedThreadPoolExecutor);
+ }
+ }
+
+ private static class MatchAllOrThrowExceptionQuery extends Query {
+
+ private final AtomicInteger numExceptionsToThrow;
+ private final Query delegate;
+ private final AtomicInteger callsToScorer;
+ private final CountDownLatch latch;
+
+ /**
+ * Throws an Exception out of the {@code scorer} method the first {@code
numExceptions} times it
+ * is called. Otherwise, it delegates all calls to the MatchAllDocsQuery.
+ *
+ * @param numExceptions number of exceptions to throw from scorer method
+ * @param callsToScorer where to record the number of times the {@code
scorer} method has been
+ * called
+ */
+ public MatchAllOrThrowExceptionQuery(
+ int numExceptions, AtomicInteger callsToScorer, CountDownLatch latch) {
+ this.numExceptionsToThrow = new AtomicInteger(numExceptions);
+ this.callsToScorer = callsToScorer;
+ this.delegate = new MatchAllDocsQuery();
+ this.latch = latch;
+ }
+
+ @Override
+ public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode,
float boost)
+ throws IOException {
+ Weight matchAllWeight = delegate.createWeight(searcher, scoreMode,
boost);
+
+ return new Weight(delegate) {
+ @Override
+ public boolean isCacheable(LeafReaderContext ctx) {
+ return matchAllWeight.isCacheable(ctx);
+ }
+
+ @Override
+ public Explanation explain(LeafReaderContext context, int doc) throws
IOException {
+ return matchAllWeight.explain(context, doc);
+ }
+
+ @Override
+ public Scorer scorer(LeafReaderContext context) throws IOException {
+ if (numExceptionsToThrow.getAndDecrement() > 0) {
+ callsToScorer.getAndIncrement();
+ try {
+ throw new RuntimeException("MatchAllOrThrowExceptionQuery
Exception");
+ } finally {
+ latch.countDown();
+ }
+ } else {
+ try {
+ latch.await(5000, TimeUnit.MILLISECONDS);
Review Comment:
maybe add a comment here to explain that the slices that don't throw will
wait for all the exceptions to be thrown before proceeding, in order to make
the test more deterministic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
