Handling BAD_REQUEST exceptions as failures even with shard.tolerant as true #139
Conversation
There was a problem hiding this comment.
A couple of minor suggestions, but overall looks good!
For potential upstreaming (which would depend on upstreaming #129) we'd probably want to check and make sure there are not other contexts in which shards.tolerant is applied, and if so make corresponding changes there.
| public int getRspCode() { | ||
| return rspCode; | ||
| } | ||
|
|
There was a problem hiding this comment.
👍 surprising that this wasn't already accessible! Seems pretty general-purpose useful.
|
|
||
| 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")); |
There was a problem hiding this comment.
curious: with a mini solr cluster of size 1, wouldn't this request have failed even before this change?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
oh, nice! so it was very tolerant before 😁
| private static final Collection<SolrException.ErrorCode> NONTOLERANT_ERROR_CODES = | ||
| List.of(SolrException.ErrorCode.BAD_REQUEST); | ||
|
|
There was a problem hiding this comment.
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.
I did a quick look on this and I think this is the only area that would need to be changed, but I can do a more thorough look as part of upstreaming. |
…as true (#139) * Handling BAD_REQUEST exceptions as failures even with shard.tolerant as true * Fixing forbiddenapi use * Updating the set of error codes to just be a Set
…as true (#139) * Handling BAD_REQUEST exceptions as failures even with shard.tolerant as true * Fixing forbiddenapi use * Updating the set of error codes to just be a Set
This will ensure that distributed requests are considered failing requests if any shard fails with a BAD_REQUEST (400) error indicating that that request is not something that can just succeed on retry.