[GitHub] [solr] cpoerschke commented on a diff in pull request #1026: SOLR-16418: SearchGroupsResultTransformer NPE during query timeout or exception

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

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


##########
solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/SearchGroupShardResponseProcessor.java:
##########
@@ -108,9 +114,11 @@ public void process(ResponseBuilder rb, ShardRequest shardRequest) {
             .put(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY, Boolean.TRUE);
         continue; // continue if there was an error and we're tolerant.
       }
-      maxElapsedTime = (int) Math.max(maxElapsedTime, srsp.getSolrResponse().getElapsedTime());
-      NamedList<NamedList<?>> firstPhaseResult =
-          (NamedList<NamedList<?>>) srsp.getSolrResponse().getResponse().get("firstPhase");
+      maxElapsedTime = (int) Math.max(maxElapsedTime, solrResponse.getElapsedTime());

Review Comment:
   minor/pre-existing: instead of `(int)` casting within the loop here the local variable could be changed to be `long` type and then only one `(int)` cast would be needed at the point of the `rb.firstPhaseElapsedTime = maxElapsedTime;` assignment



##########
solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/SearchGroupShardResponseProcessor.java:
##########
@@ -108,9 +114,11 @@ public void process(ResponseBuilder rb, ShardRequest shardRequest) {
             .put(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY, Boolean.TRUE);
         continue; // continue if there was an error and we're tolerant.
       }
-      maxElapsedTime = (int) Math.max(maxElapsedTime, srsp.getSolrResponse().getElapsedTime());
-      NamedList<NamedList<?>> firstPhaseResult =
-          (NamedList<NamedList<?>>) srsp.getSolrResponse().getResponse().get("firstPhase");
+      maxElapsedTime = (int) Math.max(maxElapsedTime, solrResponse.getElapsedTime());
+      NamedList<NamedList<?>> firstPhaseResult = getFirstPhaseFromShardResponse(rb, srsp);
+      if (firstPhaseResult == null) {

Review Comment:
   noting that `secondPhase` is already checking for null -- https://github.com/apache/solr/blob/releases/solr/9.0.0/solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/TopGroupsShardResponseProcessor.java#L126 -- though it appears to not consider the partial response flag



##########
solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/SearchGroupShardResponseProcessor.java:
##########
@@ -171,4 +174,43 @@ public void process(ResponseBuilder rb, ShardRequest shardRequest) {
       }
     }
   }
+
+  @SuppressWarnings("unchecked")
+  private static NamedList<NamedList<?>> getFirstPhaseFromShardResponse(
+      ResponseBuilder rb, ShardResponse srsp) {
+    NamedList<NamedList<?>> firstPhaseResult;
+    try {
+      SolrResponse solrResponse = srsp.getSolrResponse();
+      NamedList<?> response = solrResponse.getResponse();
+      firstPhaseResult = (NamedList<NamedList<?>>) response.get("firstPhase");
+      if (firstPhaseResult != null) {
+        return firstPhaseResult;
+      } else {
+        NamedList<?> responseHeader =
+            (NamedList<?>) response.get(SolrQueryResponse.RESPONSE_HEADER_KEY);
+        if (responseHeader.getBooleanArg(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY)) {
+          return null;
+        } else {
+          log.warn("corrupted response on {} : {}", srsp.getShardRequest(), solrResponse);
+          throw new SolrException(
+              SolrException.ErrorCode.SERVER_ERROR,
+              "firstPhase"
+                  + " is absent in response from "
+                  + srsp.getNodeName()
+                  + ", but "
+                  + SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY
+                  + " isn't set in the response.");
+        }
+      }
+    } catch (Exception ex) {
+      if (ShardParams.getShardsTolerantAsBool(rb.req.getParams())) {
+        return null;
+      } else {
+        throw new SolrException(
+            SolrException.ErrorCode.SERVER_ERROR,
+            "Unable to read facet info for shard: " + srsp.getShard(),

Review Comment:
   copy/paste from facet PR change
   ```suggestion
               "Unable to read first phase info for shard: " + srsp.getShard(),
   ```
   
   I wonder if some shared utility method somewhere could be useful for use by faceting and first and second phase grouping code, and possibly other code?
   
   ```
   static ... getSubResponseFromShardResponse(ResponseBuilder rb, ShardResponse srsp, String subResponseKey) ...
   ```



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