SOLR-17869: Avoid creating grouping shard requests when timeAllowed is exhausted (#3678)

Author: sigram

solr/CHANGES.txt

@@ -280,6 +280,8 @@ Bug Fixes
 
 * SOLR-17837: PULL replica nodes could be marked as "preferredLeader" by BALANCESHARDUNIQUE despite never being able to be elected leader (Kevin Liang via Houston Putman)
 
+* SOLR-17869: Avoid creating grouping shard requests when timeAllowed has already run out. (Andrzej Bialecki, hossman)
+
 Dependency Upgrades
 ---------------------
 (No changes)

solr/core/src/java/org/apache/solr/search/grouping/distributed/requestfactory/TopGroupsShardRequestFactory.java

@@ -83,6 +83,22 @@ private ShardRequest[] createRequest(ResponseBuilder rb, String[] shards) {
     sreq.purpose = ShardRequest.PURPOSE_GET_TOP_IDS;
     sreq.params = new ModifiableSolrParams(rb.req.getParams());
 
+    // TODO: should eventually all components that send sub-requests be subject to similar
+    //  logic if we are running with timeAllowed?
+    int origTimeAllowed = sreq.params.getInt(CommonParams.TIME_ALLOWED, -1);
+    if (origTimeAllowed > 0) {
+      int remainingTimeAllowed = origTimeAllowed - rb.firstPhaseElapsedTime;
+      // at least 1 ms to be useful
+      if (remainingTimeAllowed <= 1) {
+        // We've already used up our time budget
+        rb.rsp.setPartialResults(rb.req);
+        rb.rsp.addPartialResponseDetail(
+            getClass().getSimpleName() + ": timeAllowed exhausted in first phase");
+        return new ShardRequest[0];
+      }
+      sreq.params.set(CommonParams.TIME_ALLOWED, remainingTimeAllowed);
+    }
+
     // If group.format=simple group.offset doesn't make sense
     Grouping.Format responseFormat = rb.getGroupingSpec().getResponseFormat();
     if (responseFormat == Grouping.Format.simple || rb.getGroupingSpec().isMain()) {
@@ -132,12 +148,6 @@ private ShardRequest[] createRequest(ResponseBuilder rb, String[] shards) {
       sreq.params.set(CommonParams.FL, schema.getUniqueKeyField().getName());
     }
 
-    int origTimeAllowed = sreq.params.getInt(CommonParams.TIME_ALLOWED, -1);
-    if (origTimeAllowed > 0) {
-      sreq.params.set(
-          CommonParams.TIME_ALLOWED, Math.max(1, origTimeAllowed - rb.firstPhaseElapsedTime));
-    }
-
     return new ShardRequest[] {sreq};
   }
 }

solr/core/src/test/org/apache/solr/TestDistributedGrouping.java

@@ -28,6 +28,13 @@
 import org.apache.solr.common.params.ModifiableSolrParams;
 import org.apache.solr.common.params.SolrParams;
 import org.apache.solr.common.util.NamedList;
+import org.apache.solr.handler.component.QueryComponent;
+import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.handler.component.ShardRequest;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.search.grouping.distributed.requestfactory.TopGroupsShardRequestFactory;
+import org.junit.BeforeClass;
 import org.junit.Test;
 
 /**
@@ -39,6 +46,11 @@
 @SuppressPointFields(bugUrl = "https://issues.apache.org/jira/browse/SOLR-10844")
 public class TestDistributedGrouping extends BaseDistributedSearchTestCase {
 
+  @BeforeClass
+  public static void beforeClass() throws Exception {
+    initCore("solrconfig-delaying-component.xml", "schema.xml");
+  }
+
   public TestDistributedGrouping() {
     // SOLR-10844: Even with points suppressed, this test breaks if we (randomize) docvalues="true"
     // on trie fields?!?!?!!?
@@ -1251,19 +1263,48 @@ public void test() throws Exception {
       for (String rows : new String[] {"10", "0"}) {
         simpleQuery(
             "q", "*:*", "group", "true", "group.field", i1, "group.ngroups", ngroups, "rows", rows);
-        simpleQuery(
-            "q",
-            "*:*",
-            "group",
-            "true",
-            "group.field",
-            i1,
-            "group.ngroups",
-            ngroups,
-            "rows",
-            rows,
-            "timeAllowed",
-            "123456");
+        // delaying component introduces a delay longer than timeAllowed
+        QueryResponse rsp =
+            simpleQuery(
+                "q",
+                t1 + ":eggs",
+                "group",
+                "true",
+                "group.field",
+                i1,
+                "group.ngroups",
+                ngroups,
+                "rows",
+                rows,
+                "cache",
+                "false",
+                "timeAllowed",
+                "200",
+                "sleep",
+                "300");
+        assertTrue(
+            "header: " + rsp.getHeader(), SolrQueryResponse.isPartialResults(rsp.getHeader()));
+        //
+        rsp =
+            simpleQuery(
+                "q",
+                t1 + ":eggs",
+                "group",
+                "true",
+                "group.field",
+                i1,
+                "group.ngroups",
+                ngroups,
+                "rows",
+                rows,
+                "cache",
+                "false",
+                "timeAllowed",
+                "200",
+                "sleep",
+                "10");
+        assertFalse(
+            "header: " + rsp.getHeader(), SolrQueryResponse.isPartialResults(rsp.getHeader()));
       }
     }
 
@@ -1650,13 +1691,51 @@ public void test() throws Exception {
         "true");
   }
 
-  private void simpleQuery(Object... queryParams) throws Exception {
+  @Test
+  public void testShardRequestFactory() throws Exception {
+    SolrQueryRequest req =
+        req(
+            "q",
+            "*:*",
+            "rows",
+            "100",
+            "fl",
+            "id," + i1,
+            "group",
+            "true",
+            "group.field",
+            i1,
+            "group.limit",
+            "-1",
+            "sort",
+            i1 + " asc, id asc",
+            "timeAllowed",
+            "200");
+    try {
+      SolrQueryResponse rsp = new SolrQueryResponse();
+      ResponseBuilder rb = new ResponseBuilder(req, rsp, List.of());
+      new QueryComponent().prepare(rb);
+      rb.setNeedDocSet(true);
+
+      TopGroupsShardRequestFactory f = new TopGroupsShardRequestFactory();
+      ShardRequest[] sreq = f.constructRequest(rb);
+      assertTrue(sreq.length > 0);
+
+      rb.firstPhaseElapsedTime = 200; // simulate timeout
+      sreq = f.constructRequest(rb);
+      assertEquals(0, sreq.length);
+    } finally {
+      req.close();
+    }
+  }
+
+  private QueryResponse simpleQuery(Object... queryParams) throws Exception {
     ModifiableSolrParams params = new ModifiableSolrParams();
     for (int i = 0; i < queryParams.length; i += 2) {
       params.add(queryParams[i].toString(), queryParams[i + 1].toString());
     }
     params.set("shards", shards);
-    queryRandomShard(params);
+    return queryRandomShard(params);
   }
 
   /**