SOLR-11431: Report 503 instead of 500 for a core that fails to initialize

changelog/unreleased/SOLR-11431.yml

@@ -0,0 +1,7 @@
+title: Report 503 instead of 500 for a core that fails to initialize
+type: changed
+authors:
+  - name: Nick Shanin
+links:
+  - name: SOLR-11431
+    url: https://issues.apache.org/jira/browse/SOLR-11431

solr/core/src/java/org/apache/solr/core/CoreContainer.java

@@ -2215,7 +2215,7 @@ public SolrCore getCore(String name, UUID id) {
     // If it's not yet loaded, we can check if it's had a core init failure and "do the right thing"
     CoreDescriptor desc = solrCores.getCoreDescriptor(name);
 
-    // if there was an error initializing this core, throw a 500
+    // if there was an error initializing this core, throw a 503
     // error with the details for clients attempting to access it.
     CoreLoadFailure loadFailure = getCoreInitFailures().get(name);
     if (loadFailure != null) {

solr/core/src/java/org/apache/solr/core/SolrCoreInitializationException.java

@@ -26,7 +26,7 @@ public SolrCoreInitializationException(ErrorCode code, String msg) {
 
   public SolrCoreInitializationException(String coreName, Exception loadException) {
     super(
-        ErrorCode.SERVER_ERROR,
+        ErrorCode.SERVICE_UNAVAILABLE,
         "SolrCore '"
             + coreName
             + "' is not available due to init failure: "

solr/core/src/java/org/apache/solr/handler/component/ShardResponse.java

@@ -78,7 +78,7 @@ void setShard(String shard) {
     this.shard = shard;
   }
 
-  void setException(Throwable exception) {
+  public void setException(Throwable exception) {
     this.exception = exception;
   }
 
@@ -97,7 +97,7 @@ public String getShardAddress() {
     return this.shardAddress;
   }
 
-  void setShardAddress(String addr) {
+  public void setShardAddress(String addr) {
     this.shardAddress = addr;
   }
 }

solr/core/src/java/org/apache/solr/update/PeerSync.java

@@ -364,7 +364,8 @@ private void requestVersions(String replica) {
     shardHandler.submit(sreq, replica, sreq.params);
   }
 
-  private boolean handleResponse(ShardResponse srsp) {
+  @VisibleForTesting
+  boolean handleResponse(ShardResponse srsp) {
     ShardRequest sreq = srsp.getShardRequest();
 
     if (srsp.getException() != null) {
@@ -395,27 +396,22 @@ private boolean handleResponse(ShardResponse srsp) {
         }
       }
 
-      if (cantReachIsSuccess
-          && sreq.purpose == SHARD_REQUEST_PURPOSE_GET_VERSIONS
-          && srsp.getException() instanceof SolrException
-          && ((SolrException) srsp.getException()).code() == 503) {
-        log.warn(
-            "{} got a 503 from {}, counting as success ",
-            msg(),
-            srsp.getShardAddress(),
-            srsp.getException());
-        return true;
-      }
-
-      if (cantReachIsSuccess
-          && sreq.purpose == SHARD_REQUEST_PURPOSE_GET_VERSIONS
-          && srsp.getException() instanceof SolrException
-          && ((SolrException) srsp.getException()).code() == 404) {
-        log.warn(
-            "{} got a 404 from {}, counting as success. {} Perhaps /get is not registered?",
-            msg(),
-            srsp.getShardAddress(),
-            srsp.getException());
+      if (isToleratedLeaderElectionException(
+          srsp.getException(), cantReachIsSuccess, sreq.purpose)) {
+        int exceptionCode = ((SolrException) srsp.getException()).code();
+        if (exceptionCode == SolrException.ErrorCode.SERVICE_UNAVAILABLE.code) {
+          log.warn(
+              "{} got a 503 from {}, counting as success ",
+              msg(),
+              srsp.getShardAddress(),
+              srsp.getException());
+        } else {
+          log.warn(
+              "{} got a 404 from {}, counting as success. {} Perhaps /get is not registered?",
+              msg(),
+              srsp.getShardAddress(),
+              srsp.getException());
+        }
         return true;
       }
 
@@ -438,6 +434,24 @@ private boolean handleResponse(ShardResponse srsp) {
     }
   }
 
+  /**
+   * Returns whether a failed version request can be ignored while selecting a leader. A core that
+   * fails to initialize responds with 503 and therefore cannot participate in peer sync.
+   */
+  @VisibleForTesting
+  static boolean isToleratedLeaderElectionException(
+      Throwable exception, boolean cantReachIsSuccess, int shardRequestPurpose) {
+    if (!cantReachIsSuccess || shardRequestPurpose != SHARD_REQUEST_PURPOSE_GET_VERSIONS) {
+      return false;
+    }
+    if (!(exception instanceof SolrException)) {
+      return false;
+    }
+    int exceptionCode = ((SolrException) exception).code();
+    return exceptionCode == SolrException.ErrorCode.SERVICE_UNAVAILABLE.code
+        || exceptionCode == SolrException.ErrorCode.NOT_FOUND.code;
+  }
+
   // sometimes the root exception is a SocketTimeoutException, but ConnectTimeoutException
   // is in the chain
   private boolean connectTimeoutExceptionInChain(Throwable exception) {

solr/core/src/test/org/apache/solr/core/TestCoreContainer.java

@@ -891,14 +891,14 @@ public void testCoreInitFailuresFromEmptyContainer() throws Exception {
 
     // check that we get null accessing a non-existent core
     assertNull(cc.getCore("does_not_exist"));
-    // check that we get a 500 accessing the core with an init failure
+    // check that we get a 503 accessing the core with an init failure
     thrown =
         expectThrows(
             SolrException.class,
             () -> {
               SolrCore c = cc.getCore("bogus");
             });
-    assertEquals(500, thrown.code());
+    assertEquals(SolrException.ErrorCode.SERVICE_UNAVAILABLE.code, thrown.code());
     String cause = SolrException.getRootCause(thrown).getMessage();
     assertTrue(
         "getCore() ex cause doesn't mention init fail: " + cause, 0 < cause.indexOf("bogus_path"));
@@ -950,14 +950,14 @@ public void testCoreInitFailuresOnReload() throws Exception {
     // check that we get null accessing a non-existent core
     assertNull(cc.getCore("does_not_exist"));
     assertFalse(cc.isLoaded("does_not_exist"));
-    // check that we get a 500 accessing the core with an init failure
+    // check that we get a 503 accessing the core with an init failure
     SolrException thrown =
         expectThrows(
             SolrException.class,
             () -> {
               SolrCore c = cc.getCore("col_bad");
             });
-    assertEquals(500, thrown.code());
+    assertEquals(SolrException.ErrorCode.SERVICE_UNAVAILABLE.code, thrown.code());
     String cause = thrown.getCause().getCause().getMessage();
     assertTrue(
         "getCore() ex cause doesn't mention init fail: " + cause,
@@ -1024,14 +1024,14 @@ public void testCoreInitFailuresOnReload() throws Exception {
 
     // check that we get null accessing a non-existent core
     assertNull(cc.getCore("does_not_exist"));
-    // check that we get a 500 accessing the core with an init failure
+    // check that we get a 503 accessing the core with an init failure
     thrown =
         expectThrows(
             SolrException.class,
             () -> {
               SolrCore c = cc.getCore("bogus");
             });
-    assertEquals(500, thrown.code());
+    assertEquals(SolrException.ErrorCode.SERVICE_UNAVAILABLE.code, thrown.code());
     cause = thrown.getCause().getMessage();
     assertTrue(
         "getCore() ex cause doesn't mention init fail: " + cause, 0 < cause.indexOf("bogus_path"));

solr/core/src/test/org/apache/solr/update/PeerSyncFailedCoreResponseTest.java

@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.update;
+
+import java.util.List;
+import org.apache.solr.SolrTestCase;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.core.SolrCoreInitializationException;
+import org.apache.solr.handler.component.ShardRequest;
+import org.apache.solr.handler.component.ShardResponse;
+import org.apache.solr.util.EmbeddedSolrServerTestRule;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+
+/**
+ * Wiring tests for {@link PeerSync#handleResponse}: a response from a core that failed to
+ * initialize must flow through the real response handler and be counted as success during leader
+ * election, not just satisfy {@link PeerSync#isToleratedLeaderElectionException} in isolation.
+ * These live outside {@link PeerSyncTest} because that suite calls initCore from its constructor,
+ * so every additional test method in it leaks a harness core. The core here only exists so a
+ * PeerSync can be constructed; handleResponse itself reads nothing from it beyond the log prefix.
+ */
+public class PeerSyncFailedCoreResponseTest extends SolrTestCase {
+
+  @ClassRule
+  public static final EmbeddedSolrServerTestRule solrTestRule = new EmbeddedSolrServerTestRule();
+
+  @BeforeClass
+  public static void beforeClass() throws Exception {
+    solrTestRule.startSolr(SolrTestCaseJ4.TEST_HOME());
+    SolrTestCaseJ4.newRandomConfig();
+    solrTestRule.newCollection().withConfigSet(SolrTestCaseJ4.TEST_COLL1_CONF()).create();
+  }
+
+  @Test
+  public void testFailedCoreResponseCountsAsSuccessDuringLeaderElection() throws Exception {
+    try (SolrCore core = solrTestRule.getCoreContainer().getCore("collection1");
+        PeerSync peerSync = new PeerSync(core, List.of("http://example.com/solr/core"), 10, true)) {
+      ShardResponse response =
+          failedResponse(
+              PeerSync.SHARD_REQUEST_PURPOSE_GET_VERSIONS,
+              new SolrCoreInitializationException("failed_core", new Exception("invalid config")));
+
+      assertTrue(peerSync.handleResponse(response));
+    }
+  }
+
+  @Test
+  public void testNotFoundResponseCountsAsSuccessDuringLeaderElection() throws Exception {
+    try (SolrCore core = solrTestRule.getCoreContainer().getCore("collection1");
+        PeerSync peerSync = new PeerSync(core, List.of("http://example.com/solr/core"), 10, true)) {
+      ShardResponse response =
+          failedResponse(
+              PeerSync.SHARD_REQUEST_PURPOSE_GET_VERSIONS,
+              new SolrException(SolrException.ErrorCode.NOT_FOUND, "no such core"));
+
+      assertTrue(peerSync.handleResponse(response));
+    }
+  }
+
+  @Test
+  public void testServerErrorResponseStillFailsDuringLeaderElection() throws Exception {
+    try (SolrCore core = solrTestRule.getCoreContainer().getCore("collection1");
+        PeerSync peerSync = new PeerSync(core, List.of("http://example.com/solr/core"), 10, true)) {
+      ShardResponse response =
+          failedResponse(
+              PeerSync.SHARD_REQUEST_PURPOSE_GET_VERSIONS,
+              new SolrException(SolrException.ErrorCode.SERVER_ERROR, "boom"));
+
+      assertFalse(peerSync.handleResponse(response));
+    }
+  }
+
+  @Test
+  public void testFailedCoreResponseStillFailsForUpdateRequests() throws Exception {
+    try (SolrCore core = solrTestRule.getCoreContainer().getCore("collection1");
+        PeerSync peerSync = new PeerSync(core, List.of("http://example.com/solr/core"), 10, true)) {
+      ShardResponse response =
+          failedResponse(
+              PeerSync.SHARD_REQUEST_PURPOSE_GET_UPDATES,
+              new SolrCoreInitializationException("failed_core", new Exception("invalid config")));
+
+      assertFalse(peerSync.handleResponse(response));
+    }
+  }
+
+  @Test
+  public void testFailedCoreResponseStillFailsWhenCantReachIsNotSuccess() throws Exception {
+    try (SolrCore core = solrTestRule.getCoreContainer().getCore("collection1");
+        PeerSync peerSync =
+            new PeerSync(core, List.of("http://example.com/solr/core"), 10, false)) {
+      ShardResponse response =
+          failedResponse(
+              PeerSync.SHARD_REQUEST_PURPOSE_GET_VERSIONS,
+              new SolrCoreInitializationException("failed_core", new Exception("invalid config")));
+
+      assertFalse(peerSync.handleResponse(response));
+    }
+  }
+
+  private static ShardResponse failedResponse(int purpose, Throwable exception) {
+    ShardRequest request = new ShardRequest();
+    request.purpose = purpose;
+
+    // ShardResponse is final, so it cannot be mocked; build a real one.
+    ShardResponse response = new ShardResponse();
+    response.setShardRequest(request);
+    response.setException(exception);
+    response.setShardAddress("http://example.com/solr/core");
+    return response;
+  }
+}

solr/core/src/test/org/apache/solr/update/PeerSyncLeaderElectionTest.java

@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.update;
+
+import org.apache.solr.SolrTestCase;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.core.SolrCoreInitializationException;
+import org.junit.Test;
+
+public class PeerSyncLeaderElectionTest extends SolrTestCase {
+
+  @Test
+  public void testFailedCoreIsIgnoredDuringLeaderElectionVersionRequest() {
+    SolrCoreInitializationException failedCore =
+        new SolrCoreInitializationException("failed_core", new Exception("invalid config"));
+
+    assertEquals(SolrException.ErrorCode.SERVICE_UNAVAILABLE.code, failedCore.code());
+    assertTrue(
+        PeerSync.isToleratedLeaderElectionException(
+            failedCore, true, PeerSync.SHARD_REQUEST_PURPOSE_GET_VERSIONS));
+    assertFalse(
+        PeerSync.isToleratedLeaderElectionException(
+            new SolrException(SolrException.ErrorCode.SERVER_ERROR, "generic failure"),
+            true,
+            PeerSync.SHARD_REQUEST_PURPOSE_GET_VERSIONS));
+    assertFalse(
+        PeerSync.isToleratedLeaderElectionException(
+            failedCore, false, PeerSync.SHARD_REQUEST_PURPOSE_GET_VERSIONS));
+    assertFalse(
+        PeerSync.isToleratedLeaderElectionException(
+            failedCore, true, PeerSync.SHARD_REQUEST_PURPOSE_GET_UPDATES));
+  }
+}