This is an automated email from the ASF dual-hosted git repository. dsmiley pushed a commit to branch branch_10x in repository https://gitbox.apache.org/repos/asf/solr.git
commit db513fd3f5d791bde21b916762bcffec0fc733eb Author: Serhiy Bzhezytskyy <[email protected]> AuthorDate: Sun Aug 16 20:39:12 2026 +0300 SOLR-18346: decide update retries from the cause chain, not one fixed position in it (#4678) A transient connection failure from a shard leader to one of its replicas is now retried when the failure arrives wrapped inside another exception, instead of sending the replica into recovery. Previously whether the retry happened depended on which exception the client reported outermost. (cherry picked from commit ed893b5faaf4a89ed1dbdb8efa7990b0675a9d95) --- .../checkretry-unroll-exception-chain.yml | 10 ++ .../org/apache/solr/update/SolrCmdDistributor.java | 50 ++++--- .../apache/solr/update/CheckRetryUnrollTest.java | 151 +++++++++++++++++++++ 3 files changed, 195 insertions(+), 16 deletions(-) diff --git a/changelog/unreleased/checkretry-unroll-exception-chain.yml b/changelog/unreleased/checkretry-unroll-exception-chain.yml new file mode 100644 index 00000000000..f83740ed3f0 --- /dev/null +++ b/changelog/unreleased/checkretry-unroll-exception-chain.yml @@ -0,0 +1,10 @@ +title: > + A transient connection failure from a shard leader to one of its replicas is now retried when the + failure arrives wrapped inside another exception, instead of sending the replica into recovery. + Previously whether the retry happened depended on which exception the client reported outermost. +type: fixed +authors: + - name: Serhiy Bzhezytskyy +links: + - name: SOLR-18346 + url: https://issues.apache.org/jira/browse/SOLR-18346 diff --git a/solr/core/src/java/org/apache/solr/update/SolrCmdDistributor.java b/solr/core/src/java/org/apache/solr/update/SolrCmdDistributor.java index 40c972aefcb..ea1d2789700 100644 --- a/solr/core/src/java/org/apache/solr/update/SolrCmdDistributor.java +++ b/solr/core/src/java/org/apache/solr/update/SolrCmdDistributor.java @@ -33,7 +33,6 @@ import java.util.concurrent.CompletionService; import java.util.concurrent.ExecutorCompletionService; import java.util.concurrent.Future; import org.apache.solr.client.solrj.SolrClient; -import org.apache.solr.client.solrj.SolrServerException; import org.apache.solr.client.solrj.impl.ConcurrentUpdateBaseSolrClient; import org.apache.solr.client.solrj.request.AbstractUpdateRequest; import org.apache.solr.client.solrj.request.UpdateRequest; @@ -56,6 +55,9 @@ import org.slf4j.LoggerFactory; public class SolrCmdDistributor implements Closeable { private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); + /** Cause chains are shallow in practice; the cap only guards against a cyclic chain. */ + private static final int MAX_CAUSE_DEPTH = 100; + private StreamingSolrClients clients; private boolean finished = false; // see finish() @@ -572,23 +574,30 @@ public class SolrCmdDistributor implements Closeable { } // if it's a connect exception, lets try again - if (err.e instanceof SolrServerException) { - if (isRetriableException(((SolrServerException) err.e).getRootCause())) { - return true; - } - } else { - if (isRetriableException(err.e)) { - return true; - } - } - return false; + return isRetriableException(err.e); } /** + * Inspects the whole cause chain, because a retriable failure is not always the outermost or + * the deepest exception. The async client reports a connection failure wrapped in an + * ExecutionException, and Jetty's ClientConnector wraps the underlying failure in a + * SocketException of its own, so neither the top-level type nor the root cause alone identifies + * every retriable case. + * * @return true if Solr should retry in case of hitting this exception false otherwise */ private boolean isRetriableException(Throwable t) { - return t instanceof SocketException || t instanceof SocketTimeoutException; + // Bounded: a cause chain can be cyclic, as the TODO on SolrException.getRootCause notes. + // Real chains are a handful of frames deep. + int depth = 0; + for (Throwable cause = t; + cause != null && depth++ < MAX_CAUSE_DEPTH; + cause = cause.getCause()) { + if (cause instanceof SocketException || cause instanceof SocketTimeoutException) { + return true; + } + } + return false; } @Override @@ -644,6 +653,18 @@ public class SolrCmdDistributor implements Closeable { private ZkStateReader zkStateReader; + private static boolean hasConnectExceptionInChain(Throwable t) { + int depth = 0; + for (Throwable cause = t; + cause != null && depth++ < MAX_CAUSE_DEPTH; + cause = cause.getCause()) { + if (cause instanceof ConnectException) { + return true; + } + } + return false; + } + public ForwardNode( ZkCoreNodeProps nodeProps, ZkStateReader zkStateReader, @@ -664,10 +685,7 @@ public class SolrCmdDistributor implements Closeable { } // if it's a connect exception, lets try again - if (err.e instanceof SolrServerException - && ((SolrServerException) err.e).getRootCause() instanceof ConnectException) { - doRetry = true; - } else if (err.e instanceof ConnectException) { + if (hasConnectExceptionInChain(err.e)) { doRetry = true; } if (doRetry) { diff --git a/solr/core/src/test/org/apache/solr/update/CheckRetryUnrollTest.java b/solr/core/src/test/org/apache/solr/update/CheckRetryUnrollTest.java new file mode 100644 index 00000000000..fc49ae46578 --- /dev/null +++ b/solr/core/src/test/org/apache/solr/update/CheckRetryUnrollTest.java @@ -0,0 +1,151 @@ +/* + * 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.io.IOException; +import java.net.ConnectException; +import java.net.SocketException; +import java.nio.channels.ClosedChannelException; +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.ExecutionException; +import org.apache.solr.SolrTestCase; +import org.apache.solr.client.solrj.SolrServerException; +import org.apache.solr.common.cloud.Replica; +import org.apache.solr.common.cloud.ZkCoreNodeProps; +import org.junit.Test; + +/** + * Whether a retriable failure is retried should not depend on which exception is outermost. + * + * <p>Covers {@link SolrCmdDistributor.StdNode}. {@link SolrCmdDistributor.ForwardNode} carries the + * same asymmetry and is changed the same way, but needs a live ZkStateReader to construct, so it + * stays covered by SolrCmdDistributorTest rather than here. + */ +public class CheckRetryUnrollTest extends SolrTestCase { + + private static Replica replica() { + Map<String, Object> props = new HashMap<>(); + props.put("base_url", "http://127.0.0.1:8983/solr";); + props.put("core", "collection1"); + props.put("node_name", "127.0.0.1:8983_solr"); + props.put("type", "NRT"); + props.put("state", "active"); + return new Replica("core_node1", props, "collection1", "shard1"); + } + + private static SolrCmdDistributor.Node node() { + return new SolrCmdDistributor.StdNode( + new ZkCoreNodeProps(replica()), "collection1", "shard1", /* maxRetries= */ 1); + } + + private static boolean retries(Exception e) { + SolrCmdDistributor.SolrError err = new SolrCmdDistributor.SolrError(); + err.e = e; + return node().checkRetry(err); + } + + @Test + public void testRetriesWhenSocketExceptionIsWrappedInSolrServerException() { + // the shape checkRetry already unwraps + assertTrue( + retries(new SolrServerException("wrapped", new SocketException("Connection reset")))); + } + + @Test + public void testRetriesWhenSocketExceptionIsTopLevel() { + assertTrue(retries(new SocketException("Connection reset"))); + } + + @Test + public void testRetriesWhenSocketExceptionIsWrappedInSomethingElse() { + // the async (Jetty) path delivers a connection failure as an ExecutionException; the socket + // cause is just as retriable as in the two cases above, but the outer type is not + // SolrServerException so the leaf test never sees it. + assertTrue(retries(new ExecutionException(new ConnectException("Connection refused")))); + } + + @Test + public void testRetriesWhenSocketExceptionIsNestedDeeply() { + assertTrue( + retries( + new ExecutionException( + new RuntimeException("io", new SocketException("Connection reset"))))); + } + + @Test + public void testDoesNotRetryOnAServerErrorRootedInSomethingElse() { + // control: the SolrServerException shape that already worked must keep its answer + assertFalse(retries(new SolrServerException("wrapped", new IllegalStateException("nope")))); + } + + @Test + public void testClosedChannelExceptionIsStillNotRetriableEitherWay() { + // Documents a limit of this change rather than a fix. ClosedChannelException is what the JDK + // transport actually reports as the root cause of a dropped update connection, and it is not a + // SocketException, so it stays non-retriable however the chain is inspected. Widening + // isRetriableException is a separate behaviour decision. + assertFalse(retries(new ExecutionException(new ClosedChannelException()))); + assertFalse(retries(new SolrServerException("wrapped", new ClosedChannelException()))); + } + + @Test + public void testAnUnretriableNodeNeverRetriesHoweverTheChainLooks() { + // The count ceiling lives in Req.shouldRetry, not here, but checkRetry has its own gate: a node + // built with maxRetries=0 has retry==false and must refuse before the exception is even looked + // at. Unrolling must not bypass that. + SolrCmdDistributor.Node noRetries = + new SolrCmdDistributor.StdNode(new ZkCoreNodeProps(replica()), "collection1", "shard1"); + SolrCmdDistributor.SolrError err = new SolrCmdDistributor.SolrError(); + err.e = new ExecutionException(new ConnectException("Connection refused")); + assertFalse(noRetries.checkRetry(err)); + } + + @Test + public void testRetriesWhenTheRetriableTypeIsNotTheRootCause() { + // Jetty's ClientConnector wraps the underlying failure in a SocketException of its own + // (ClientConnector#connect), so the retriable frame can sit above the root cause. Going + // straight + // to the root cause would miss it. + assertTrue( + retries( + new ExecutionException( + new SocketException("Could not connect to host", new IOException("underlying"))))); + } + + @Test + public void testTerminatesOnACyclicCauseChain() { + // A cause chain can be made cyclic, which is why the scan is bounded -- see the TODO on + // SolrException#getRootCause. This must return rather than spin. + Exception first = new Exception("first"); + Exception second = new Exception("second", first); + try { + first.initCause(second); + } catch (IllegalStateException | IllegalArgumentException alreadySet) { + // some JDKs refuse; nothing to assert then + return; + } + assertFalse(retries(second)); + } + + @Test + public void testDoesNotRetryWhenNothingInTheChainIsRetriable() { + // control: unrolling must not make everything retriable + assertFalse(retries(new ExecutionException(new IllegalStateException("not retriable")))); + assertFalse(retries(new IllegalArgumentException("not retriable"))); + } +}
