[GitHub] [lucene-solr] madrob commented on a change in pull request #1436: SOLR-14413: allow timeAllowed and cursorMark parameters

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

madrob commented on a change in pull request #1436:
URL: https://github.com/apache/lucene-solr/pull/1436#discussion_r473126349



##########
File path: solr/core/src/test/org/apache/solr/CursorPagingTest.java
##########
@@ -499,6 +492,61 @@ public void testSimple() throws Exception {
                               ));
   }
 
+  /**
+   * test that timeAllowed parameter can be used with cursors
+   * uses DelayingSearchComponent in solrconfig-deeppaging.xml
+   */
+  public void testTimeAllowed() throws Exception {
+    String wontExceedTimeout = "10000";
+    int numDocs = 100;
+    // Create a bunch of docs, inspired by createIndex in ExitableDirectoryReaderTest
+    for (int i = 0; i < numDocs; i++) {
+      assertU(adoc("id", Integer.toString(i), "name", "a" + i + " b" + i + " c" + i + " d"+i + " e" + i));

Review comment:
       This doesn't have to be so complex, we can use `"name", "a" + i` I think?

##########
File path: solr/core/src/test/org/apache/solr/CursorPagingTest.java
##########
@@ -499,6 +492,61 @@ public void testSimple() throws Exception {
                               ));
   }
 
+  /**
+   * test that timeAllowed parameter can be used with cursors
+   * uses DelayingSearchComponent in solrconfig-deeppaging.xml
+   */
+  public void testTimeAllowed() throws Exception {
+    String wontExceedTimeout = "10000";
+    int numDocs = 100;
+    // Create a bunch of docs, inspired by createIndex in ExitableDirectoryReaderTest
+    for (int i = 0; i < numDocs; i++) {
+      assertU(adoc("id", Integer.toString(i), "name", "a" + i + " b" + i + " c" + i + " d"+i + " e" + i));
+      if (random().nextInt(numDocs) == 0) {
+        assertU(commit());  // sometimes make multiple segments
+      }
+    }
+    assertU(commit());
+
+    String cursorMark;
+    SolrParams params = null;
+
+    // start by triggering partial results - set the timeAllowed
+    // lower than the sleep used in DelayingSearchComponent
+    // NB: this only seems to work reliably if results havent
+    // been returned yet.
+    SolrParams partialParams = params("q", "name:a*",
+        "fl", "id",
+        "sort", "id desc",
+        "rows", "2",
+        "sleep", "10",
+        "timeAllowed", "1");
+
+    cursorMark = CURSOR_MARK_START;
+
+    // assertCursor will confirm the nextCursorMark was set
+    String partialCursorMark = assertCursor(req(partialParams, CURSOR_MARK_PARAM, cursorMark)
+        , "/responseHeader/partialResults==true]"
+    );
+
+    // we can continue on, paging as normal
+    cursorMark = partialCursorMark;
+    params = params("q", "*:*",
+        "fl", "id",
+        "sort", "id desc",
+        "rows", "2",
+        "timeAllowed", wontExceedTimeout);
+    cursorMark = assertCursor(req(params, CURSOR_MARK_PARAM, cursorMark)
+        , "/response/numFound==100"
+    );
+    assertNotEquals(cursorMark, partialCursorMark);
+
+    cursorMark = assertCursor(req(params, CURSOR_MARK_PARAM, cursorMark)

Review comment:
       this assignment is not used. Maybe we call another query and make sure we get the same, i.e. we're at the end?

##########
File path: solr/solr-ref-guide/src/common-query-parameters.adoc
##########
@@ -206,7 +206,7 @@ The default value of this parameter is blank, which causes no extra "explain inf
 
 == timeAllowed Parameter
 
-This parameter specifies the amount of time, in milliseconds, allowed for a search to complete. If this time expires before the search is complete, any partial results will be returned, but values such as `numFound`, <<faceting.adoc#faceting,facet>> counts, and result <<the-stats-component.adoc#the-stats-component,stats>> may not be accurate for the entire result set. In case of expiration, if `omitHeader` isn't set to `true` the response header contains a special flag called `partialResults`.
+This parameter specifies the amount of time, in milliseconds, allowed for a search to complete. If this time expires before the search is complete, any partial results will be returned, but values such as `numFound`, <<pagination-of-results.adoc#using-cursors,`nextCursorMark`>>, <<faceting.adoc#faceting,facet>> counts, and result <<the-stats-component.adoc#the-stats-component,stats>> may not be accurate for the entire result set. In case of expiration, if `omitHeader` isn't set to `true` the response header contains a special flag called `partialResults`.

Review comment:
       I don't think it's correct to say that nextCursorMark will be inaccurate. It will be accurate for the next result that should be returned.
   
   This line also makes me think that maybe `timeAllowed+cursorMark+omitHeader` shouldn't be a valid combination. It's not a regression because `timeAllowed+cursorMark` is already disallowed. WDYT?

##########
File path: solr/core/src/test/org/apache/solr/CursorPagingTest.java
##########
@@ -499,6 +492,61 @@ public void testSimple() throws Exception {
                               ));
   }
 
+  /**
+   * test that timeAllowed parameter can be used with cursors
+   * uses DelayingSearchComponent in solrconfig-deeppaging.xml
+   */
+  public void testTimeAllowed() throws Exception {
+    String wontExceedTimeout = "10000";
+    int numDocs = 100;
+    // Create a bunch of docs, inspired by createIndex in ExitableDirectoryReaderTest
+    for (int i = 0; i < numDocs; i++) {
+      assertU(adoc("id", Integer.toString(i), "name", "a" + i + " b" + i + " c" + i + " d"+i + " e" + i));

Review comment:
       Alternatively, that method is `public static`, reuse it so that we don't have this logic duplicated all over the place.

##########
File path: solr/solr-ref-guide/src/pagination-of-results.adoc
##########
@@ -97,6 +97,7 @@ There are a few important constraints to be aware of when using `cursorMark` par
 
 . `cursorMark` and `start` are mutually exclusive parameters.
 * Your requests must either not include a `start` parameter, or it must be specified with a value of "```0```".
+. When using the <<common-query-parameters.adoc#timeallowed-parameter,`timeAllowed` request param>>, partial results may be returned.  If time expires before the search is complete - as indicated when the `responseHeader` includes `"partialResults": true` - and `cursorMark` matches `nextCursorMark`, you cannot be sure that there are no more results. In this situation, consider increasing `timeAllowed` and reissuing the query.  When the `responseHeader` no longer includes `"partialResults": true` and `cursorMark` matches `nextCursorMark`, there are no more results.

Review comment:
       👍 




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

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