[GitHub] [lucene] msokolov commented on a diff in pull request #927: LUCENE-10151: Adding Timeout Support to IndexSearcher

[GitBox <gi...@apache.org>]

msokolov commented on code in PR #927:
URL: https://github.com/apache/lucene/pull/927#discussion_r885712790


##########
lucene/core/src/java/org/apache/lucene/search/TimeLimitingBulkScorer.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.lucene.search;
+
+import java.io.IOException;
+import org.apache.lucene.index.QueryTimeoutImpl;
+import org.apache.lucene.util.Bits;
+
+/**
+ * The {@link TimeLimitingCollector} is used to timeout search requests that take longer than the
+ * maximum allowed search time limit. After this time is exceeded, the search thread is stopped by
+ * throwing a {@link TimeLimitingCollector.TimeExceededException}.
+ *
+ * @see org.apache.lucene.index.ExitableDirectoryReader
+ */
+public class TimeLimitingBulkScorer extends BulkScorer {
+  /** Thrown when elapsed search time exceeds allowed search time. */
+  @SuppressWarnings("serial")
+  public static class TimeExceededException extends RuntimeException {
+
+    private TimeExceededException() {
+      super("TimeLimit Exceeded");
+    }
+  }
+
+  private BulkScorer in;
+  private QueryTimeoutImpl queryTimeout;
+
+  public TimeLimitingBulkScorer(BulkScorer bulkScorer, QueryTimeoutImpl queryTimeout) {
+    this.in = bulkScorer;
+    this.queryTimeout = queryTimeout;
+  }
+
+  @Override
+  public int score(LeafCollector collector, Bits acceptDocs, int min, int max) throws IOException {
+    int interval = 100;
+    while (min < max) {
+      final int newMax = (int) Math.min((long) min + interval, max);
+      if (queryTimeout.shouldExit() == true) {
+        throw new TimeLimitingBulkScorer.TimeExceededException();
+      }
+      min = in.score(collector, acceptDocs, min, newMax); // in is the wrapped bulk scorer
+    }
+    return min;
+  }
+
+  @Override
+  public long cost() {
+    return 0;

Review Comment:
   Should we return in.score()?



##########
lucene/core/src/test/org/apache/lucene/search/TestTimeLimitingBulkScorer.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.lucene.search;
+
+import org.apache.lucene.document.Document;
+import org.apache.lucene.document.Field;
+import org.apache.lucene.index.*;
+import org.apache.lucene.store.Directory;
+import org.apache.lucene.tests.analysis.MockAnalyzer;
+import org.apache.lucene.tests.util.LuceneTestCase;
+
+/** Tests the {@link TimeLimitingBulkScorer}. */
+@LuceneTestCase.SuppressSysoutChecks(
+    bugUrl = "http://test.is.timing.sensitive.so.it.prints.instead.of.failing")

Review Comment:
   I don't think we need the print statement? It's enough for the test to succeed, doesn't need to print anything. And you have an assert?



##########
lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java:
##########
@@ -32,15 +32,7 @@
 import java.util.concurrent.ThreadPoolExecutor;
 import java.util.function.Supplier;
 import org.apache.lucene.document.Document;
-import org.apache.lucene.index.DirectoryReader;
-import org.apache.lucene.index.IndexReader;
-import org.apache.lucene.index.IndexReaderContext;
-import org.apache.lucene.index.IndexWriter;
-import org.apache.lucene.index.LeafReaderContext;
-import org.apache.lucene.index.ReaderUtil;
-import org.apache.lucene.index.StoredFieldVisitor;
-import org.apache.lucene.index.Term;
-import org.apache.lucene.index.Terms;
+import org.apache.lucene.index.*;

Review Comment:
   Let's not use star-imports. Maybe your IDE is automatically converting?



##########
lucene/core/src/test/org/apache/lucene/search/TestTimeLimitingBulkScorer.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.lucene.search;
+
+import org.apache.lucene.document.Document;
+import org.apache.lucene.document.Field;
+import org.apache.lucene.index.*;
+import org.apache.lucene.store.Directory;
+import org.apache.lucene.tests.analysis.MockAnalyzer;
+import org.apache.lucene.tests.util.LuceneTestCase;
+
+/** Tests the {@link TimeLimitingBulkScorer}. */
+@LuceneTestCase.SuppressSysoutChecks(
+    bugUrl = "http://test.is.timing.sensitive.so.it.prints.instead.of.failing")
+public class TestTimeLimitingBulkScorer extends LuceneTestCase {
+
+  public void testTimeLimitingBulkScorer() throws Exception {
+    Directory directory = newDirectory();
+    IndexWriter writer =
+        new IndexWriter(directory, newIndexWriterConfig(new MockAnalyzer(random())));
+    int n = 10000;
+    for (int i = 0; i < n; i++) {
+      Document d = new Document();
+      d.add(newTextField("default", "ones ", Field.Store.YES));
+      writer.addDocument(d);
+    }
+    writer.forceMerge(1);
+    writer.commit();
+    writer.close();
+
+    DirectoryReader directoryReader;
+    IndexSearcher searcher;
+    TopDocs top;
+    ScoreDoc[] hits = null;
+
+    Query query = new TermQuery(new Term("default", "ones"));
+    directoryReader = DirectoryReader.open(directory);
+    searcher = new IndexSearcher(directoryReader);
+    top = searcher.search(query, n, 5);
+
+    if (top != null) {
+      hits = top.scoreDocs;
+      assertTrue("Partial result", hits.length > 0 && hits.length < n);

Review Comment:
   Timing tests are tricky! This seems rather fragile. If there is a hiccup might we get zero results? Also we had to index a lot of documents in order to run this test. I see that in TestExitableDirectoryReader we pass in a QueryTimeout that can be overridden with special implementations in the test. Perhaps we should have an API here that accepts a QueryTimeout? And we can provide a factory method on the interface (QueryTimeout.milliseconds(int n)) that returns a QueryTimeoutImpl, for users. 



-- 
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-unsubscribe@lucene.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org