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.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 @@ 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 boolean checkRetry(SolrError err) { } // 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 static class ForwardNode extends StdNode { 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 boolean checkRetry(SolrError err) { } // 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. + * + *

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 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"))); + } +}