You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/09/19 17:56:10 UTC
[GitHub] [solr] risdenk opened a new pull request, #1026: SOLR-16418: SearchGroupsResultTransformer NPE during query timeout or exception
risdenk opened a new pull request, #1026:
URL: https://github.com/apache/solr/pull/1026
https://issues.apache.org/jira/browse/SOLR-16418
During a query exception or timeout, there can be an NPE getting the firstPhase results. This extracts a method to deal with it. The commit also does some minor cleanup at the same time to avoid unnecessary casts.
--
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
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1026: SOLR-16418: SearchGroupsResultTransformer NPE during query timeout or exception
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1026:
URL: https://github.com/apache/solr/pull/1026#discussion_r974703951
##########
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)) {
Review Comment:
*NULL_DEREFERENCE:* object returned by `responseHeader.getBooleanArg("partialResults")` could be null and is dereferenced at line 191.
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=334293336&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=334293336&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=334293336&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=334293336&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=334293336&lift_comment_rating=5) ]
##########
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)) {
Review Comment:
*NULL_DEREFERENCE:* object `responseHeader` last assigned on line 190 could be null and is dereferenced at line 191.
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=334293566&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=334293566&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=334293566&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=334293566&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=334293566&lift_comment_rating=5) ]
--
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
[GitHub] [solr] risdenk commented on a diff in pull request #1026: SOLR-16418: SearchGroupsResultTransformer NPE during query timeout or exception
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1026:
URL: https://github.com/apache/solr/pull/1026#discussion_r975504508
##########
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:
Added `SolrResponseUtil` and tried to address a bunch of other places this could come up.
--
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
[GitHub] [solr] risdenk merged pull request #1026: SOLR-16418: Introduce SolrResponseUtil to handle NPE during query timeout or exception when parsing SolrResponse
Posted by GitBox <gi...@apache.org>.
risdenk merged PR #1026:
URL: https://github.com/apache/solr/pull/1026
--
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
[GitHub] [solr] cpoerschke commented on a diff in pull request #1026: SOLR-16418: SearchGroupsResultTransformer NPE during query timeout or exception
Posted by 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
[GitHub] [solr] risdenk commented on pull request #1026: SOLR-16418: Introduce SolrResponseUtil to handle NPE during query timeout or exception when parsing SolrResponse
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #1026:
URL: https://github.com/apache/solr/pull/1026#issuecomment-1252801457
Force pushed to simplify all my intermediate commits and fix the changes/commit message.
--
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
[GitHub] [solr] risdenk commented on a diff in pull request #1026: SOLR-16418: SearchGroupsResultTransformer NPE during query timeout or exception
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1026:
URL: https://github.com/apache/solr/pull/1026#discussion_r975506424
##########
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:
Addressed this.
--
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
[GitHub] [solr] risdenk commented on a diff in pull request #1026: SOLR-16418: SearchGroupsResultTransformer NPE during query timeout or exception
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1026:
URL: https://github.com/apache/solr/pull/1026#discussion_r975417709
##########
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:
Yea you came to the same conclusion I did. I'll see what I can do to find a place to put this.
--
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
[GitHub] [solr] risdenk commented on a diff in pull request #1026: SOLR-16418: SearchGroupsResultTransformer NPE during query timeout or exception
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1026:
URL: https://github.com/apache/solr/pull/1026#discussion_r975504086
##########
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:
Thanks added util class to help with these. I addressed a bunch of other places that follow the same pattern.
--
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