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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
