SOLR-17206: Don't respond with status=-1 (#2368)
It is possible for a response status code of -1 to be used for an update request that is distributed, and when all distributed requests fail with an exception that is not a SolrException (e.g. IOException)
Co-authored-by: Paul McArthur <pmcarthur-apache@proton.me>
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index c48284e..94c269e 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -152,7 +152,7 @@
* SOLR-17198: AffinityPlacementFactory can fail if Shard leadership changes occur while it is collecting metrics.
(Paul McArthur)
-
+
* SOLR-17018: Add QueryLimits support to Learning To Rank rescoring.
(Alessandro Benedetti)
@@ -169,6 +169,9 @@
* SOLR-17218: Fix indexFetcher logging to include MDC details (hossman)
+* SOLR-17206: Eliminate the possibility of a -1 status code for SolrCloud update requests that are distributed.
+ (Paul McArthur)
+
Dependency Upgrades
---------------------
diff --git a/solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java b/solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java
index 48d637d..8588a0a 100644
--- a/solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java
+++ b/solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java
@@ -1289,8 +1289,8 @@
maxCode = Math.max(error.statusCode, maxCode);
}
if (minCode == maxCode) {
- // all codes are consistent, use that...
- return minCode;
+ // all codes are consistent, use that, as long as it's not the default value of -1
+ return minCode != -1 ? minCode : ErrorCode.SERVER_ERROR.code;
} else if (400 <= minCode && maxCode < 500) {
// all codes are 4xx, use 400
return ErrorCode.BAD_REQUEST.code;
diff --git a/solr/core/src/test/org/apache/solr/update/processor/DistributedUpdateProcessorTest.java b/solr/core/src/test/org/apache/solr/update/processor/DistributedUpdateProcessorTest.java
index a1289db..dc43043 100644
--- a/solr/core/src/test/org/apache/solr/update/processor/DistributedUpdateProcessorTest.java
+++ b/solr/core/src/test/org/apache/solr/update/processor/DistributedUpdateProcessorTest.java
@@ -24,6 +24,7 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.List;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
@@ -37,6 +38,7 @@
import org.apache.solr.request.SolrQueryRequest;
import org.apache.solr.update.AddUpdateCommand;
import org.apache.solr.update.DeleteUpdateCommand;
+import org.apache.solr.update.SolrCmdDistributor;
import org.apache.solr.update.TimedVersionBucket;
import org.apache.solr.update.UpdateLog;
import org.apache.solr.update.VersionInfo;
@@ -137,6 +139,26 @@
assertThat(succeeded, is(threads));
}
+ @Test
+ public void testStatusCodeOnDistribError_NotSolrException() {
+
+ // SolrCmdDistributor defaults to a status code of -1, and sets it to a legal value only if
+ // the distributed exception is a SolrException instance. If it isn't it remains as -1,
+ // which should be replaced with a 500
+ final String message = "some communication issue";
+ SolrCmdDistributor.SolrError e = new SolrCmdDistributor.SolrError();
+ e.e = new IOException(message);
+
+ DistributedUpdateProcessor.DistributedUpdatesAsyncException distribError =
+ new DistributedUpdateProcessor.DistributedUpdatesAsyncException(List.of(e));
+ assertEquals(
+ "Expected HTTP 500 status code for distributed update IOException",
+ 500,
+ distribError.code());
+ assertEquals(
+ "Async exception during distributed update: " + message, distribError.getMessage());
+ }
+
/**
* @return how many requests succeeded
*/
