[GitHub] [solr] cpoerschke commented on a diff in pull request #1450: SOLR-16693: timeAllowed: use more efficient TimeLimitingBulkScorer

["cpoerschke (via GitHub)" <gi...@apache.org>]

cpoerschke commented on code in PR #1450:
URL: https://github.com/apache/solr/pull/1450#discussion_r1136792660


##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -713,6 +713,33 @@ public QueryResult search(QueryResult qr, QueryCommand cmd) throws IOException {
     return qr;
   }
 
+  @Override
+  protected void search(List<LeafReaderContext> leaves, Weight weight, Collector collector)
+      throws IOException {
+    final var queryTimeout = SolrQueryTimeoutImpl.getInstance();
+    if (queryTimeout.isTimeoutEnabled() == false) {
+      // no timeout.  Pass through to subclass

Review Comment:
   ```suggestion
         // no timeout.  Pass through to super class
   ```



##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -713,6 +713,33 @@ public QueryResult search(QueryResult qr, QueryCommand cmd) throws IOException {
     return qr;
   }
 
+  @Override
+  protected void search(List<LeafReaderContext> leaves, Weight weight, Collector collector)
+      throws IOException {
+    final var queryTimeout = SolrQueryTimeoutImpl.getInstance();
+    if (queryTimeout.isTimeoutEnabled() == false) {
+      // no timeout.  Pass through to subclass
+      super.search(leaves, weight, collector);
+    } else {
+      // Timeout enabled!  This impl is maybe a hack.  Use Lucene's IndexSearcher timeout.
+      // But only some queries have it so don't use on "this" (SolrIndexSearcher), not to mention
+      //   that timedOut() might race with concurrent queries (dubious design).
+      // So we need to make a new IndexSearcher instead of using "this".
+      new IndexSearcher(reader) { // cheap, actually!
+        void searchWithTimeout() throws IOException {
+          setTimeout(queryTimeout.makeLocalImpl());
+          super.search(leaves, weight, collector); // FYI protected access
+          if (timedOut()) {
+            // We might not even be using ExitableDirectoryReader yet various places in Solr check
+            //  for this exception.  Sometimes others but this seems most suitable.
+            throw new ExitableDirectoryReader.ExitingReaderException(
+                "Timed out.  Not actually via ExitableDirectoryReader");

Review Comment:
   Maybe an alternative might be to have a `class SolrNotActuallyExitableDirectoryReader extends ExitableDirectoryReader.ExitingReaderException` approach which would be thrown here and then over time the various places could be switched over to check for `SolrNotActuallyExitableDirectoryReader` instead or to have the checks phased out.



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

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


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