(solr) branch main updated: SOLR-17206: Don't respond with status=-1 (#2368)

[dsmiley]

This is an automated email from the ASF dual-hosted git repository.

dsmiley pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/solr.git


The following commit(s) were added to refs/heads/main by this push:
     new 4d57b44dbc1 SOLR-17206: Don't respond with status=-1 (#2368)
4d57b44dbc1 is described below

commit 4d57b44dbc161d1ea9d3ee38e5099ea59ee2dc93
Author: pjmcarthur <92818583+pjmcart...@users.noreply.github.com>
AuthorDate: Wed Apr 10 14:43:41 2024 +0200

    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-apa...@proton.me>
---
 solr/CHANGES.txt                                   |  5 ++++-
 .../processor/DistributedUpdateProcessor.java      |  4 ++--
 .../processor/DistributedUpdateProcessorTest.java  | 22 ++++++++++++++++++++++
 3 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index c48284e9e19..94c269e3ffb 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -152,7 +152,7 @@ Bug Fixes
 
 * 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 @@ Bug Fixes
 
 * 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 48d637d3a39..8588a0a34de 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 @@ public class DistributedUpdateProcessor extends 
UpdateRequestProcessor {
         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 a1289db36a7..dc430437121 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 static org.mockito.Mockito.doReturn;
 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.LocalSolrQueryRequest;
 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 @@ public class DistributedUpdateProcessorTest extends 
SolrTestCaseJ4 {
     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
    */