Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handling BAD_REQUEST exceptions as failures even with shard.tolerant as true #139

Merged
merged 3 commits into from
Sep 7, 2023
Merged

Handling BAD_REQUEST exceptions as failures even with shard.tolerant as true #139

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1c9ef2d
Handling BAD_REQUEST exceptions as failures even with shard.tolerant …
Sep 6, 2023
2409b12
Fixing forbiddenapi use
Sep 6, 2023
202769e
Updating the set of error codes to just be a Set
Sep 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.lang.invoke.MethodHandles;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.Iterator;
Expand Down Expand Up @@ -92,6 +93,9 @@ public class SearchHandler extends RequestHandlerBase

protected static final String SHARD_HANDLER_SUFFIX = "[shard]";

private static final Collection<SolrException.ErrorCode> NONTOLERANT_ERROR_CODES =
List.of(SolrException.ErrorCode.BAD_REQUEST);

Copy link
Collaborator

@magibney magibney Sep 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just make this a Set -- i.e. Set<> X = Set.of() -- avoid the extra import. For a list of size 1 it's not going to matter much, but you're doing "set-like things" on this anyway, so there's little purpose served in going broader and making it a Collection.

private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());

/** A counter to ensure that no RID is equal, even if they fall in the same millisecond */
Expand Down Expand Up @@ -572,7 +576,9 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
if (srsp.getException() != null) {
log.warn("Shard request failed : {}", srsp);
// If things are not tolerant, abort everything and rethrow
if (!tolerant) {
if (!tolerant
|| NONTOLERANT_ERROR_CODES.contains(
SolrException.ErrorCode.getErrorCode(srsp.getRspCode()))) {
shardHandler1.cancelAll();
if (srsp.getException() instanceof SolrException) {
throw (SolrException) srsp.getException();
Expand Down
4 changes: 4 additions & 0 deletions solr/core/src/java/org/apache/solr/handler/component/ShardResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ void setResponseCode(int rspCode) {
this.rspCode = rspCode;
}

public int getRspCode() {
return rspCode;
}

Comment on lines +84 to +87
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 surprising that this wasn't already accessible! Seems pretty general-purpose useful.

void setNodeName(String nodeName) {
this.nodeName = nodeName;
}
Expand Down
13 changes: 13 additions & 0 deletions solr/core/src/test/org/apache/solr/handler/component/SearchHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,19 @@ public void testLuceneIOExceptionHandling() throws Exception {
assertTrue(
e.getMessage()
.contains("Query contains too many nested clauses; maxClauseCount is set to 1"));

solrQuery = new SolrQuery("{!surround maxBasicQueries=1000 df=title}10W(tes*,test*)");
solrQuery.add(ShardParams.SHARDS_TOLERANT, "true");
final QueryRequest req4 = new QueryRequest(solrQuery);
req4.setMethod(SolrRequest.METHOD.POST);
e =
assertThrows(
BaseHttpSolrClient.RemoteSolrException.class,
() -> req4.process(cloudSolrClient, collectionName));
assertEquals(400, e.code());
assertTrue(
e.getMessage()
.contains("Query contains too many nested clauses; maxClauseCount is set to 1"));
Comment on lines +363 to +375
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious: with a mini solr cluster of size 1, wouldn't this request have failed even before this change?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even with the cluster of size 1, it still flows through the distributed query section of the SearchHandler and previously did not result in an error, but would just return no results and partial results as true.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, nice! so it was very tolerant before 😁

} finally {
if (initialMaxBooleanClauses != null) {
System.setProperty("solr.max.booleanClauses", initialMaxBooleanClauses);
Expand Down
Loading