[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_r895817658


##########
build.gradle:
##########
@@ -183,3 +183,5 @@ apply from: file('gradle/hacks/turbocharge-jvm-opts.gradle')
 apply from: file('gradle/hacks/dummy-outputs.gradle')
 
 apply from: file('gradle/pylucene/pylucene.gradle')
+sourceCompatibility = JavaVersion.VERSION_17

Review Comment:
   why did we need to add this?



##########
lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java:
##########
@@ -532,6 +536,11 @@ public TopDocs reduce(Collection<TopScoreDocCollector> collectors) throws IOExce
     return search(query, manager);
   }
 
+  public void setTimeout(boolean isTimeoutEnabled, QueryTimeout queryTimeout) throws IOException {

Review Comment:
   Could we use `queryTimeout==null` or a sentinel value `QueryTime.NONE` to indicate no timeout is enabled? It would save a redundant parameter and member variable. Actually I see QueryTimeout has a timeoutEnabled() method, so could we define NONE to return false and just check that in our branches instead of this separate boolean flag?



##########
lucene/core/src/java/org/apache/lucene/search/TimeLimitingBulkScorer.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.QueryTimeout;
+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 {
+
+  static final int INTERVAL = 100;

Review Comment:
   please add a comment for this constant - what is it used for? Actually we should describe the algorithm here; namely that we score chunks of documents at a time so as to avoid the cost of checking the timeout for every document we score



##########
lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java:
##########
@@ -766,18 +778,29 @@ protected void search(List<LeafReaderContext> leaves, Weight weight, Collector c
       }
       BulkScorer scorer = weight.bulkScorer(ctx);
       if (scorer != null) {
-        try {
-          scorer.score(leafCollector, ctx.reader().getLiveDocs());
-        } catch (
-            @SuppressWarnings("unused")
-            CollectionTerminatedException e) {
-          // collection was terminated prematurely
-          // continue with the following leaf
+        if (isTimeoutEnabled) {
+          TimeLimitingBulkScorer timeLimitingBulkScorer =
+              new TimeLimitingBulkScorer(scorer, queryTimeout);
+          try {
+            timeLimitingBulkScorer.score(leafCollector, ctx.reader().getLiveDocs());
+          } catch (
+              @SuppressWarnings("unused")
+              TimeLimitingBulkScorer.TimeExceededException e) {
+            partialResult = true;

Review Comment:
   I wonder if we should use this as a way to provide some information to the caller, for example how much time elapsed when the timeout occurred? The exception could pass that back? On the other hand, then every QueryTimeout might have to track that, and for some of them (eg counting-based) the time isn't really the most important dimension.



##########
lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java:
##########
@@ -555,6 +564,9 @@ public void search(Query query, Collector results) throws IOException {
     search(leafContexts, createWeight(query, results.scoreMode(), 1), results);
   }
 
+  public boolean isAborted() {

Review Comment:
   How about `timedOut()` ? It will be more symmetric with the methods/variables using timeout in their names.



##########
lucene/core/src/java/org/apache/lucene/index/ExitableDirectoryReader.java:
##########
@@ -82,8 +81,8 @@ public PointValues getPointValues(String field) throws IOException {
         return null;
       }
       return (queryTimeout.isTimeoutEnabled())
-          ? new ExitablePointValues(pointValues, queryTimeout)
-          : pointValues;
+              ? new ExitablePointValues(pointValues, queryTimeout)

Review Comment:
   why are we changing indentation? Isn't spotless enforcing this for us?



-- 
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