From 0fecae71210433d1298dd5ab3ac317b194b273c0 Mon Sep 17 00:00:00 2001 From: chan-dx Date: Fri, 28 Aug 2026 17:16:57 +0100 Subject: [PATCH 01/20] SOLR-18402: SolrJ transports classify their own failures --- .../apache/solr/client/solrj/SolrClient.java | 17 ++++++++++++ .../client/solrj/impl/CloudSolrClient.java | 3 ++- .../client/solrj/impl/HttpJdkSolrClient.java | 8 ++++++ .../client/solrj/impl/HttpSolrClient.java | 17 ++++++++++++ .../solrj/impl/HttpJdkSolrClientTest.java | 26 +++++++++++++++++++ 5 files changed, 70 insertions(+), 1 deletion(-) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/SolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/SolrClient.java index 5d54d12155a..246f7b45e78 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/SolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/SolrClient.java @@ -1194,6 +1194,23 @@ public final NamedList request(final SolrRequest request) return request(request, null); } + /** + * Whether the failure proves the request never reached the server, making a replay safe even when + * the request isn't idempotent. Only the transport can answer this; the default is {@code false}, + * meaning "cannot tell" rather than "the request was sent". + */ + public boolean wasRequestUnsent(Throwable t) { + return false; + } + + /** + * Whether this is a transport-level communication failure rather than a response from the server. + * Implementations must keep {@link #wasRequestUnsent} a subset of this. + */ + public boolean wasCommError(Throwable t) { + return false; + } + /** * This method defines the context in which this Solr client is being used (e.g. for internal * communication between Solr nodes or as an external client). The default value is {@code diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java index 07c8af0c5cd..b76084c3e8e 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java @@ -210,7 +210,8 @@ public ClusterState getClusterState() { * Is this a communication error? We will retry if so. The whole cause chain is inspected, since a * transport may report the underlying failure wrapped at any depth. */ - protected boolean wasCommError(Throwable t) { + @Override + public boolean wasCommError(Throwable t) { return SolrException.hasCause(t, SocketException.class) || SolrException.hasCause(t, UnknownHostException.class) || SolrException.hasCause(t, RequestNotSentException.class); diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java index 07f809e6002..2036cd74549 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java @@ -29,6 +29,7 @@ import java.net.URI; import java.net.URISyntaxException; import java.net.http.HttpClient; +import java.net.http.HttpConnectTimeoutException; import java.net.http.HttpRequest; import java.net.http.HttpResponse; import java.net.http.HttpTimeoutException; @@ -227,6 +228,13 @@ public NamedList request(SolrRequest solrRequest, String collection) return requestWithBaseUrl(null, solrRequest, collection); } + /** A connect timeout means the connection was never established, so nothing was written. */ + @Override + public boolean wasRequestUnsent(Throwable t) { + return super.wasRequestUnsent(t) + || SolrException.hasCause(t, HttpConnectTimeoutException.class); + } + protected PreparedRequest prepareRequest( String overrideBaseUrl, SolrRequest solrRequest, String collection) throws SolrServerException, IOException { diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java index e79555c7bda..59beb013abc 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java @@ -22,7 +22,10 @@ import java.io.InputStream; import java.lang.invoke.MethodHandles; import java.lang.reflect.Constructor; +import java.net.ConnectException; import java.net.MalformedURLException; +import java.net.SocketException; +import java.net.UnknownHostException; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.util.Base64; @@ -36,6 +39,7 @@ import java.util.function.BiConsumer; import java.util.function.Function; import org.apache.solr.client.solrj.RemoteSolrException; +import org.apache.solr.client.solrj.RequestNotSentException; import org.apache.solr.client.solrj.SolrClient; import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrServerException; @@ -358,6 +362,19 @@ public Set getUrlParamNames() { return urlParamNames; } + @Override + public boolean wasRequestUnsent(Throwable t) { + return SolrException.hasCause(t, RequestNotSentException.class) + || SolrException.hasCause(t, ConnectException.class); + } + + @Override + public boolean wasCommError(Throwable t) { + return SolrException.hasCause(t, SocketException.class) + || SolrException.hasCause(t, UnknownHostException.class) + || wasRequestUnsent(t); + } + /** * @lucene.internal */ diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java index 9f9233f375e..eb2b3640c6b 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java @@ -18,13 +18,17 @@ package org.apache.solr.client.solrj.impl; import java.io.IOException; +import java.net.ConnectException; import java.net.CookieHandler; import java.net.CookieManager; import java.net.ServerSocket; import java.net.Socket; +import java.net.SocketException; import java.net.URI; import java.net.URISyntaxException; +import java.net.UnknownHostException; import java.net.http.HttpClient; +import java.net.http.HttpConnectTimeoutException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -41,6 +45,7 @@ import org.apache.lucene.util.NamedThreadFactory; import org.apache.solr.client.api.util.SolrVersion; import org.apache.solr.client.solrj.RemoteSolrException; +import org.apache.solr.client.solrj.RequestNotSentException; import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrServerException; import org.apache.solr.client.solrj.request.JavaBinRequestWriter; @@ -735,6 +740,27 @@ private HttpJdkSolrClient.Builder builder(String url) { return builder(url, DEFAULT_CONNECTION_TIMEOUT, DEFAULT_CONNECTION_TIMEOUT); } + @Test + public void testErrorClassification() throws Exception { + String url = solrTestRule.getBaseUrl() + DEBUG_SERVLET_PATH; + try (HttpJdkSolrClient client = builder(url).build()) { + IOException unsent = + new RequestNotSentException("Broken pipe", new IOException("Broken pipe")); + assertTrue(client.wasRequestUnsent(new SolrServerException("wrapped", unsent))); + assertTrue(client.wasRequestUnsent(new ConnectException("Connection refused"))); + assertTrue(client.wasRequestUnsent(new HttpConnectTimeoutException("timed out"))); + + // A bare IOException may have been sent and applied, so it is never proof of the contrary. + assertFalse(client.wasRequestUnsent(new IOException("Broken pipe"))); + assertFalse(client.wasRequestUnsent(new UnknownHostException("nosuchhost"))); + + assertTrue(client.wasCommError(new UnknownHostException("nosuchhost"))); + assertTrue(client.wasCommError(new SocketException("Connection reset"))); + assertTrue(client.wasCommError(new HttpConnectTimeoutException("timed out"))); + assertFalse(client.wasCommError(new IOException("Broken pipe"))); + } + } + private byte[] javabinResponse() { String[] str = JAVABIN_STR.split(" "); byte[] bytes = new byte[str.length]; From cde6efaa709d4c5591206a03a7eaa94aad886052 Mon Sep 17 00:00:00 2001 From: chan-dx Date: Fri, 28 Aug 2026 17:18:51 +0100 Subject: [PATCH 02/20] SOLR-18402: CloudSolrClient asks its transport if it was a comm error --- .../solr/client/solrj/impl/CloudSolrClient.java | 17 +++++++---------- .../solrj/impl/CloudSolrClientCacheTest.java | 2 +- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java index b76084c3e8e..c1933584865 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java @@ -21,8 +21,6 @@ import java.io.IOException; import java.lang.invoke.MethodHandles; -import java.net.SocketException; -import java.net.UnknownHostException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -48,7 +46,6 @@ import java.util.concurrent.locks.ReentrantLock; import java.util.function.Supplier; import java.util.stream.Collectors; -import org.apache.solr.client.solrj.RequestNotSentException; import org.apache.solr.client.solrj.SolrClient; import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrRequest.SolrRequestType; @@ -206,15 +203,15 @@ public ClusterState getClusterState() { return getClusterStateProvider().getClusterState(); } - /** - * Is this a communication error? We will retry if so. The whole cause chain is inspected, since a - * transport may report the underlying failure wrapped at any depth. - */ + /** Is this a communication error? We will retry if so. Answered by the underlying transport. */ @Override public boolean wasCommError(Throwable t) { - return SolrException.hasCause(t, SocketException.class) - || SolrException.hasCause(t, UnknownHostException.class) - || SolrException.hasCause(t, RequestNotSentException.class); + return getHttpClient().wasCommError(t); + } + + @Override + public boolean wasRequestUnsent(Throwable t) { + return getHttpClient().wasRequestUnsent(t); } @Override diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java index f5b29e44ab2..9494869ce84 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java @@ -431,7 +431,7 @@ public ClusterStateProvider getClusterStateProvider() { @Override public HttpSolrClient getHttpClient() { - throw new UnsupportedOperationException(); + return mock(HttpSolrClient.class); } @FunctionalInterface From 5ca97fbca982ebf4eafca478637e532380f6cb48 Mon Sep 17 00:00:00 2001 From: chan-dx Date: Fri, 28 Aug 2026 17:20:49 +0100 Subject: [PATCH 03/20] SOLR-18402: LBSolrClient asks the transport instead of matching class names --- .../client/solrj/impl/LBAsyncSolrClient.java | 10 +++------- .../solr/client/solrj/impl/LBSolrClient.java | 18 ++---------------- .../impl/LBSolrClientRetryUnsentTest.java | 7 +++++++ 3 files changed, 12 insertions(+), 23 deletions(-) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBAsyncSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBAsyncSolrClient.java index 88657cbca14..5d9d59ee0f0 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBAsyncSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBAsyncSolrClient.java @@ -17,7 +17,6 @@ package org.apache.solr.client.solrj.impl; import java.io.IOException; -import java.net.ConnectException; import java.net.SocketException; import java.net.SocketTimeoutException; import java.util.concurrent.CompletableFuture; @@ -25,7 +24,6 @@ import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicReference; import org.apache.solr.client.solrj.RemoteSolrException; -import org.apache.solr.client.solrj.RequestNotSentException; import org.apache.solr.client.solrj.SolrClient; import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrRequest.SolrRequestType; @@ -203,7 +201,7 @@ private void onFailedRequest( listener.onFailure(e, false); } } catch (SocketException e) { - if (!isNonRetryable || e instanceof ConnectException) { + if (!isNonRetryable || getClient(endpoint).wasRequestUnsent(e)) { listener.onFailure((!isZombie) ? makeServerAZombie(endpoint, e) : e, true); } else { listener.onFailure(e, false); @@ -219,9 +217,7 @@ private void onFailedRequest( if (!isNonRetryable && (rootCause instanceof IOException || rootCause instanceof TimeoutException)) { listener.onFailure((!isZombie) ? makeServerAZombie(endpoint, e) : e, true); - } else if (isNonRetryable - && (isConnectException(rootCause) - || SolrException.hasCause(e, RequestNotSentException.class))) { + } else if (isNonRetryable && getClient(endpoint).wasRequestUnsent(e)) { // Nothing of the request reached the server, so replaying it elsewhere is safe even though // it isn't idempotent. listener.onFailure((!isZombie) ? makeServerAZombie(endpoint, e) : e, true); @@ -229,7 +225,7 @@ private void onFailedRequest( listener.onFailure(e, false); } } catch (IOException e) { - if (!isNonRetryable || isConnectException(e) || e instanceof RequestNotSentException) { + if (!isNonRetryable || getClient(endpoint).wasRequestUnsent(e)) { listener.onFailure((!isZombie) ? makeServerAZombie(endpoint, e) : e, true); } else { listener.onFailure(e, false); diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java index 4a9d63cd375..2265637cea3 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java @@ -20,10 +20,8 @@ import java.io.IOException; import java.lang.invoke.MethodHandles; import java.lang.ref.WeakReference; -import java.net.ConnectException; import java.net.SocketException; import java.net.SocketTimeoutException; -import java.net.http.HttpConnectTimeoutException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -43,7 +41,6 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Collectors; import org.apache.solr.client.solrj.RemoteSolrException; -import org.apache.solr.client.solrj.RequestNotSentException; import org.apache.solr.client.solrj.SolrClient; import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrRequest.SolrRequestType; @@ -656,7 +653,7 @@ protected Exception doRequest( throw e; } } catch (SocketException e) { - if (!isNonRetryable || e instanceof ConnectException) { + if (!isNonRetryable || getClient(baseUrl).wasRequestUnsent(e)) { ex = (!isZombie) ? makeServerAZombie(baseUrl, e) : e; } else { throw e; @@ -672,9 +669,7 @@ protected Exception doRequest( if (!isNonRetryable && (rootCause instanceof IOException || rootCause instanceof TimeoutException)) { ex = (!isZombie) ? makeServerAZombie(baseUrl, e) : e; - } else if (isNonRetryable - && (isConnectException(rootCause) - || SolrException.hasCause(e, RequestNotSentException.class))) { + } else if (isNonRetryable && getClient(baseUrl).wasRequestUnsent(e)) { // Nothing of the request reached the server, so replaying it elsewhere is safe even though // it isn't idempotent. ex = (!isZombie) ? makeServerAZombie(baseUrl, e) : e; @@ -688,15 +683,6 @@ protected Exception doRequest( return ex; } - protected boolean isConnectException(Throwable t) { - if (t instanceof ConnectException || t instanceof HttpConnectTimeoutException) { - return true; - } - // Check for common connection timeout exceptions by name to avoid hard dependencies on - // specific HTTP client libraries (e.g., Jetty or Apache HttpClient). - return t != null && t.getClass().getName().endsWith("ConnectTimeoutException"); - } - protected abstract SolrClient getClient(Endpoint endpoint); private void startAliveCheckExecutor() { diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBSolrClientRetryUnsentTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBSolrClientRetryUnsentTest.java index 3163616e272..2347918df29 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBSolrClientRetryUnsentTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBSolrClientRetryUnsentTest.java @@ -27,6 +27,7 @@ import org.apache.solr.client.solrj.SolrServerException; import org.apache.solr.client.solrj.request.QueryRequest; import org.apache.solr.client.solrj.request.UpdateRequest; +import org.apache.solr.common.SolrException; import org.apache.solr.common.util.NamedList; import org.junit.Test; @@ -55,6 +56,12 @@ private static class FailFirstEndpoint extends LBSolrClient { @Override protected SolrClient getClient(Endpoint endpoint) { return new SolrClient() { + // Stands in for a transport; the LB asks it rather than inspecting the exception itself. + @Override + public boolean wasRequestUnsent(Throwable t) { + return SolrException.hasCause(t, RequestNotSentException.class); + } + @Override public NamedList request(SolrRequest request, String collection) throws SolrServerException, IOException { From fa16dbd1a913e2bc163e3a28587517a411218420 Mon Sep 17 00:00:00 2001 From: chan-dx Date: Fri, 28 Aug 2026 17:36:10 +0100 Subject: [PATCH 04/20] SOLR-18402: CloudSolrClient only replays an update the transport proves unsent --- .../client/solrj/impl/CloudSolrClient.java | 7 ++- .../solrj/impl/CloudSolrClientCacheTest.java | 46 ++++++++++++++++++- 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java index c1933584865..e2ffc530171 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java @@ -721,6 +721,10 @@ protected NamedList requestWithRetryOnStaleState( : SolrException.ErrorCode.UNKNOWN.code; final boolean wasCommError = wasCommError(exc); + // An update may already have been applied; only replay one the transport proves never + // arrived. + final boolean mayReplay = + request.getRequestType() != SolrRequestType.UPDATE || wasRequestUnsent(exc); if (wasCommError || (exc instanceof RouteException @@ -752,7 +756,8 @@ protected NamedList requestWithRetryOnStaleState( } } } - if (retryCount < MAX_STALE_RETRIES) { // if it is a communication error , we must try again + // if it is a communication error , we must try again + if (mayReplay && retryCount < MAX_STALE_RETRIES) { // may be, we have a stale version of the collection state, // and we could not get any information from the server // it is probably not worth trying again and again because diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java index 9494869ce84..a98001abdad 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java @@ -121,7 +121,7 @@ protected LBSolrClient createOrGetLbClient(HttpSolrClient myClient) { return new ConnectException("TEST"); } if (i == 2) { - return new SocketException("TEST"); + return new ConnectException("TEST"); } if (i == 3) { return new ConnectException("TEST"); @@ -138,6 +138,50 @@ protected LBSolrClient createOrGetLbClient(HttpSolrClient myClient) { } } + /** + * An update may already have been applied by the time a communication error surfaces, so it is + * replayed only when the transport proves the request never arrived. + */ + public void testUpdateIsNotReplayedWhenItMayHaveBeenApplied() throws Exception { + String collName = "gettingstarted"; + Set livenodes = new HashSet<>(); + Map refs = new HashMap<>(); + Map colls = new HashMap<>(); + + Map> responses = new HashMap<>(); + LBJettySolrClient mockLbclient = getMockLbHttpSolrClient(responses); + AtomicInteger lbhttpRequestCount = new AtomicInteger(); + try (ClusterStateProvider clusterStateProvider = getStateProvider(livenodes, refs); + CloudSolrClient cloudClient = + new RandomizingCloudSolrClientBuilder(clusterStateProvider) { + @Override + protected LBSolrClient createOrGetLbClient(HttpSolrClient myClient) { + return mockLbclient; + } + }.build()) { + livenodes.addAll(Set.of("192.168.1.108:7574_solr", "192.168.1.108:8983_solr")); + refs.put(collName, new ClusterState.CollectionRef(loadCollection(collName, 1))); + colls.put(collName, loadCollection(collName, 1)); + + // Not a ConnectException: the transport cannot prove this request never left. + responses.put( + "request", + o -> { + lbhttpRequestCount.incrementAndGet(); + return new SocketException("TEST"); + }); + UpdateRequest update = new UpdateRequest().add("id", "123", "desc", "Something 0"); + + // The routing randomization decides whether the failure arrives wrapped, so match the cause. + Exception thrown = expectThrows(Exception.class, () -> cloudClient.request(update, collName)); + assertTrue( + "the transport's failure must reach the caller", + SolrException.hasCause(thrown, SocketException.class)); + assertEquals( + "an update that may have been applied must not be replayed", 1, lbhttpRequestCount.get()); + } + } + public void testStaleStateRetrySkipsStateVersionBeforeWait() throws Exception { String collName = "gettingstarted"; Set liveNodes = new HashSet<>(Set.of("192.168.1.108:8983_solr")); From 866b1cd86302a58cdb9ee48ef9e0f86c13a63353 Mon Sep 17 00:00:00 2001 From: chan-dx Date: Fri, 28 Aug 2026 17:39:34 +0100 Subject: [PATCH 05/20] SOLR-18402: HttpJettySolrClient classifies its own transport failures --- .../solrj/jetty/HttpJettySolrClient.java | 10 +++++++ .../solrj/jetty/HttpJettySolrClientTest.java | 26 +++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java b/solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java index cf05fdac07e..ef724fead1b 100644 --- a/solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java +++ b/solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java @@ -22,6 +22,7 @@ import java.lang.invoke.MethodHandles; import java.lang.reflect.InvocationTargetException; import java.net.ConnectException; +import java.nio.channels.ClosedChannelException; import java.util.ArrayList; import java.util.Iterator; import java.util.List; @@ -49,6 +50,7 @@ import org.apache.solr.client.solrj.request.RequestWriter; import org.apache.solr.client.solrj.response.ResponseParser; import org.apache.solr.client.solrj.util.ClientUtils; +import org.apache.solr.common.SolrException; import org.apache.solr.common.params.CommonParams; import org.apache.solr.common.params.ModifiableSolrParams; import org.apache.solr.common.util.EnvUtils; @@ -82,6 +84,7 @@ import org.eclipse.jetty.http2.client.HTTP2Client; import org.eclipse.jetty.http2.client.transport.HttpClientTransportOverHTTP2; import org.eclipse.jetty.io.ClientConnector; +import org.eclipse.jetty.io.EofException; import org.eclipse.jetty.util.ssl.KeyStoreScanner; import org.eclipse.jetty.util.ssl.SslContextFactory; import org.slf4j.Logger; @@ -560,6 +563,13 @@ public R requestWithBaseUrl( } } + @Override + public boolean wasCommError(Throwable t) { + return super.wasCommError(t) + || SolrException.hasCause(t, EofException.class) + || SolrException.hasCause(t, ClosedChannelException.class); + } + @Override protected LBSolrClient createLBSolrClient() { return new LBJettySolrClient.Builder(this).build(); diff --git a/solr/solrj-jetty/src/test/org/apache/solr/client/solrj/jetty/HttpJettySolrClientTest.java b/solr/solrj-jetty/src/test/org/apache/solr/client/solrj/jetty/HttpJettySolrClientTest.java index 6a3be472677..12e11dfd7ac 100644 --- a/solr/solrj-jetty/src/test/org/apache/solr/client/solrj/jetty/HttpJettySolrClientTest.java +++ b/solr/solrj-jetty/src/test/org/apache/solr/client/solrj/jetty/HttpJettySolrClientTest.java @@ -22,14 +22,17 @@ import java.io.IOException; import java.io.InputStream; +import java.nio.channels.ClosedChannelException; import java.nio.charset.StandardCharsets; import java.util.Base64; +import java.util.List; import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import java.util.stream.IntStream; import org.apache.lucene.tests.util.LuceneTestCase; import org.apache.solr.client.api.util.SolrVersion; import org.apache.solr.client.solrj.RemoteSolrException; +import org.apache.solr.client.solrj.RequestNotSentException; import org.apache.solr.client.solrj.SolrClient; import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrServerException; @@ -55,6 +58,7 @@ import org.apache.solr.util.ServletFixtures.DebugServlet; import org.eclipse.jetty.client.WWWAuthenticationProtocolHandler; import org.eclipse.jetty.http.HttpStatus; +import org.eclipse.jetty.io.EofException; import org.eclipse.jetty.util.ssl.SslContextFactory; import org.junit.Test; @@ -774,6 +778,28 @@ public void testRequestTimeoutWithHttpClient() throws Exception { } } + @Test + public void testErrorClassification() throws Exception { + String url = solrTestRule.getBaseUrl() + DEBUG_SERVLET_PATH; + try (HttpJettySolrClient client = + (HttpJettySolrClient) builder(url, DEFAULT_CONNECTION_TIMEOUT, 0).build()) { + IOException unsent = + new RequestNotSentException("Broken pipe", new IOException("Broken pipe")); + assertTrue(client.wasRequestUnsent(new SolrServerException("wrapped", unsent))); + assertTrue(client.wasCommError(new SolrServerException("wrapped", unsent))); + + // Jetty's own connection-lost types are communication errors, but they say nothing about + // whether the request was delivered, so they must never claim it was unsent. + for (Throwable lost : + List.of(new EofException("Connection reset by peer"), new ClosedChannelException())) { + assertTrue(lost.getClass().getName(), client.wasCommError(lost)); + assertFalse(lost.getClass().getName(), client.wasRequestUnsent(lost)); + } + + assertFalse(client.wasCommError(new IOException("Broken pipe"))); + } + } + private static void assertIsTimeout(Throwable t) { assertThat(t.getMessage(), containsStringIgnoringCase("Timeout")); } From fe84321921f867aa4ff10d767bcd697e622e8851 Mon Sep 17 00:00:00 2001 From: chan-dx Date: Fri, 28 Aug 2026 17:40:27 +0100 Subject: [PATCH 06/20] SOLR-18402: Update changelog --- .../SOLR-18402-consolidate-retry-classification.yml | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 changelog/unreleased/SOLR-18402-consolidate-retry-classification.yml diff --git a/changelog/unreleased/SOLR-18402-consolidate-retry-classification.yml b/changelog/unreleased/SOLR-18402-consolidate-retry-classification.yml new file mode 100644 index 00000000000..73a7c38c08d --- /dev/null +++ b/changelog/unreleased/SOLR-18402-consolidate-retry-classification.yml @@ -0,0 +1,10 @@ +title: > + SolrJ: transports now classify their own failures via SolrClient.wasRequestUnsent / + wasCommError; CloudSolrClient and LBSolrClient ask instead of matching exception types. + CloudSolrClient now replays an update only when the transport proves it was never sent. +type: changed +authors: + - name: Han Chan +links: + - name: SOLR-18402 + url: https://issues.apache.org/jira/browse/SOLR-18402 From b3c033f164a9b0d7153388cfbfbfeab5b38e27b8 Mon Sep 17 00:00:00 2001 From: chan-dx Date: Sat, 29 Aug 2026 19:26:13 +0100 Subject: [PATCH 07/20] SOLR-18402: LBSolrClient: fail over on a bare IOException, as the async client does --- ...18402-consolidate-retry-classification.yml | 3 +- .../solr/client/solrj/impl/LBSolrClient.java | 7 ++++ .../impl/LBSolrClientRetryUnsentTest.java | 35 +++++++++++++++++++ 3 files changed, 44 insertions(+), 1 deletion(-) diff --git a/changelog/unreleased/SOLR-18402-consolidate-retry-classification.yml b/changelog/unreleased/SOLR-18402-consolidate-retry-classification.yml index 73a7c38c08d..ec7167ca29b 100644 --- a/changelog/unreleased/SOLR-18402-consolidate-retry-classification.yml +++ b/changelog/unreleased/SOLR-18402-consolidate-retry-classification.yml @@ -1,7 +1,8 @@ title: > SolrJ: transports now classify their own failures via SolrClient.wasRequestUnsent / wasCommError; CloudSolrClient and LBSolrClient ask instead of matching exception types. - CloudSolrClient now replays an update only when the transport proves it was never sent. + CloudSolrClient now replays an update only when the transport proves it was never sent, and + LBSolrClient fails over on a bare IOException instead of aborting. type: changed authors: - name: Han Chan diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java index 2265637cea3..526fd47a676 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java @@ -676,6 +676,13 @@ protected Exception doRequest( } else { throw e; } + } catch (IOException e) { + // A transport may throw one directly rather than wrapping it in a SolrServerException. + if (!isNonRetryable || getClient(baseUrl).wasRequestUnsent(e)) { + ex = (!isZombie) ? makeServerAZombie(baseUrl, e) : e; + } else { + throw e; + } } catch (Exception e) { throw new SolrServerException(e); } diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBSolrClientRetryUnsentTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBSolrClientRetryUnsentTest.java index 2347918df29..572b0e8bd0d 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBSolrClientRetryUnsentTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBSolrClientRetryUnsentTest.java @@ -126,4 +126,39 @@ public void testQueryIsStillRetriedOnAnyIOException() throws Exception { List.of(DEAD_HOST_1.getBaseUrl(), DEAD_HOST_2.getBaseUrl()), requestReturningAttemptedUrls(maybeSentException(), new QueryRequest())); } + + /** + * A transport may throw an {@link IOException} directly rather than wrapping it in a {@link + * SolrServerException}, as HttpJdkSolrClient does. LBAsyncSolrClient has always handled that; the + * synchronous path used to let it reach the catch-all and abort with no failover. + */ + @Test + public void testQueryIsRetriedOnBareIOException() throws Exception { + assertEquals( + List.of(DEAD_HOST_1.getBaseUrl(), DEAD_HOST_2.getBaseUrl()), + requestReturningAttemptedUrls(new IOException("Broken pipe"), new QueryRequest())); + } + + @Test + public void testUpdateIsNotRetriedOnBareIOException() { + LBSolrClient.Req req = + new LBSolrClient.Req(new UpdateRequest().add("id", "1"), List.of(DEAD_HOST_1, DEAD_HOST_2)); + try (FailFirstEndpoint client = new FailFirstEndpoint(new IOException("Broken pipe"))) { + expectThrows(IOException.class, () -> client.request(req)); + assertEquals(List.of(DEAD_HOST_1.getBaseUrl()), client.attempted); + } + } + + /** + * Parity with LBAsyncSolrClient, which already retried a bare {@link RequestNotSentException}. + */ + @Test + public void testUpdateIsRetriedOnBareRequestNotSentException() throws Exception { + IOException onTheWire = new IOException("Broken pipe"); + assertEquals( + List.of(DEAD_HOST_1.getBaseUrl(), DEAD_HOST_2.getBaseUrl()), + requestReturningAttemptedUrls( + new RequestNotSentException(onTheWire.getMessage(), onTheWire), + new UpdateRequest().add("id", "1"))); + } } From 7ff15ff34003061f944a3b72d260a02cdc1906e1 Mon Sep 17 00:00:00 2001 From: chan-dx Date: Sat, 29 Aug 2026 19:29:23 +0100 Subject: [PATCH 08/20] SOLR-18402: Move failure classification tests off the Jetty & Jdk fixture Collect both transports' cases in one SolrTestCase against a dead URL, so the classification is checked without booting anything. The negative cases are the point: a bare IOException and a post-commit EofException are communication failures that prove nothing about delivery. --- .../solrj/jetty/HttpJettySolrClientTest.java | 26 ---- .../solrj/impl/HttpJdkSolrClientTest.java | 26 ---- .../SolrClientErrorClassificationTest.java | 123 ++++++++++++++++++ 3 files changed, 123 insertions(+), 52 deletions(-) create mode 100644 solr/solrj/src/test/org/apache/solr/client/solrj/impl/SolrClientErrorClassificationTest.java diff --git a/solr/solrj-jetty/src/test/org/apache/solr/client/solrj/jetty/HttpJettySolrClientTest.java b/solr/solrj-jetty/src/test/org/apache/solr/client/solrj/jetty/HttpJettySolrClientTest.java index 12e11dfd7ac..6a3be472677 100644 --- a/solr/solrj-jetty/src/test/org/apache/solr/client/solrj/jetty/HttpJettySolrClientTest.java +++ b/solr/solrj-jetty/src/test/org/apache/solr/client/solrj/jetty/HttpJettySolrClientTest.java @@ -22,17 +22,14 @@ import java.io.IOException; import java.io.InputStream; -import java.nio.channels.ClosedChannelException; import java.nio.charset.StandardCharsets; import java.util.Base64; -import java.util.List; import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import java.util.stream.IntStream; import org.apache.lucene.tests.util.LuceneTestCase; import org.apache.solr.client.api.util.SolrVersion; import org.apache.solr.client.solrj.RemoteSolrException; -import org.apache.solr.client.solrj.RequestNotSentException; import org.apache.solr.client.solrj.SolrClient; import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrServerException; @@ -58,7 +55,6 @@ import org.apache.solr.util.ServletFixtures.DebugServlet; import org.eclipse.jetty.client.WWWAuthenticationProtocolHandler; import org.eclipse.jetty.http.HttpStatus; -import org.eclipse.jetty.io.EofException; import org.eclipse.jetty.util.ssl.SslContextFactory; import org.junit.Test; @@ -778,28 +774,6 @@ public void testRequestTimeoutWithHttpClient() throws Exception { } } - @Test - public void testErrorClassification() throws Exception { - String url = solrTestRule.getBaseUrl() + DEBUG_SERVLET_PATH; - try (HttpJettySolrClient client = - (HttpJettySolrClient) builder(url, DEFAULT_CONNECTION_TIMEOUT, 0).build()) { - IOException unsent = - new RequestNotSentException("Broken pipe", new IOException("Broken pipe")); - assertTrue(client.wasRequestUnsent(new SolrServerException("wrapped", unsent))); - assertTrue(client.wasCommError(new SolrServerException("wrapped", unsent))); - - // Jetty's own connection-lost types are communication errors, but they say nothing about - // whether the request was delivered, so they must never claim it was unsent. - for (Throwable lost : - List.of(new EofException("Connection reset by peer"), new ClosedChannelException())) { - assertTrue(lost.getClass().getName(), client.wasCommError(lost)); - assertFalse(lost.getClass().getName(), client.wasRequestUnsent(lost)); - } - - assertFalse(client.wasCommError(new IOException("Broken pipe"))); - } - } - private static void assertIsTimeout(Throwable t) { assertThat(t.getMessage(), containsStringIgnoringCase("Timeout")); } diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java index eb2b3640c6b..9f9233f375e 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java @@ -18,17 +18,13 @@ package org.apache.solr.client.solrj.impl; import java.io.IOException; -import java.net.ConnectException; import java.net.CookieHandler; import java.net.CookieManager; import java.net.ServerSocket; import java.net.Socket; -import java.net.SocketException; import java.net.URI; import java.net.URISyntaxException; -import java.net.UnknownHostException; import java.net.http.HttpClient; -import java.net.http.HttpConnectTimeoutException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -45,7 +41,6 @@ import org.apache.lucene.util.NamedThreadFactory; import org.apache.solr.client.api.util.SolrVersion; import org.apache.solr.client.solrj.RemoteSolrException; -import org.apache.solr.client.solrj.RequestNotSentException; import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrServerException; import org.apache.solr.client.solrj.request.JavaBinRequestWriter; @@ -740,27 +735,6 @@ private HttpJdkSolrClient.Builder builder(String url) { return builder(url, DEFAULT_CONNECTION_TIMEOUT, DEFAULT_CONNECTION_TIMEOUT); } - @Test - public void testErrorClassification() throws Exception { - String url = solrTestRule.getBaseUrl() + DEBUG_SERVLET_PATH; - try (HttpJdkSolrClient client = builder(url).build()) { - IOException unsent = - new RequestNotSentException("Broken pipe", new IOException("Broken pipe")); - assertTrue(client.wasRequestUnsent(new SolrServerException("wrapped", unsent))); - assertTrue(client.wasRequestUnsent(new ConnectException("Connection refused"))); - assertTrue(client.wasRequestUnsent(new HttpConnectTimeoutException("timed out"))); - - // A bare IOException may have been sent and applied, so it is never proof of the contrary. - assertFalse(client.wasRequestUnsent(new IOException("Broken pipe"))); - assertFalse(client.wasRequestUnsent(new UnknownHostException("nosuchhost"))); - - assertTrue(client.wasCommError(new UnknownHostException("nosuchhost"))); - assertTrue(client.wasCommError(new SocketException("Connection reset"))); - assertTrue(client.wasCommError(new HttpConnectTimeoutException("timed out"))); - assertFalse(client.wasCommError(new IOException("Broken pipe"))); - } - } - private byte[] javabinResponse() { String[] str = JAVABIN_STR.split(" "); byte[] bytes = new byte[str.length]; diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/SolrClientErrorClassificationTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/SolrClientErrorClassificationTest.java new file mode 100644 index 00000000000..d9ea47391ef --- /dev/null +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/SolrClientErrorClassificationTest.java @@ -0,0 +1,123 @@ +/* + * 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.client.solrj.impl; + +import java.io.IOException; +import java.net.ConnectException; +import java.net.SocketException; +import java.net.UnknownHostException; +import java.net.http.HttpConnectTimeoutException; +import java.nio.channels.ClosedChannelException; +import org.apache.solr.SolrTestCase; +import org.apache.solr.client.solrj.RequestNotSentException; +import org.apache.solr.client.solrj.SolrClient; +import org.apache.solr.client.solrj.SolrRequest; +import org.apache.solr.client.solrj.SolrServerException; +import org.apache.solr.client.solrj.jetty.HttpJettySolrClient; +import org.apache.solr.common.util.NamedList; +import org.eclipse.jetty.io.EofException; +import org.junit.Test; + +/** + * {@link SolrClient#wasRequestUnsent} and {@link SolrClient#wasCommError} are pure functions of the + * failure, so each transport's answers can be asserted directly rather than raced for through an + * integration test. No server is needed; the clients are never asked to send anything. + * + *

The negative cases matter most: {@code wasRequestUnsent} returning false means "cannot tell", + * and treating a failure as unsent when it isn't would replay a non-idempotent update. + */ +public class SolrClientErrorClassificationTest extends SolrTestCase { + + private static final String DEAD_URL = "http://127.0.0.1:1/solr"; + + private static SolrServerException wrapped(Throwable cause) { + return new SolrServerException("wrapped", cause); + } + + private static RequestNotSentException unsent() { + return new RequestNotSentException("Broken pipe", new IOException("Broken pipe")); + } + + /** Every HTTP transport inherits these from {@link HttpSolrClient}. */ + private static void assertSharedHttpClassification(HttpSolrClient client) { + // The transport stated the answer; it holds whether it is the failure or nested inside one. + assertTrue(client.wasRequestUnsent(unsent())); + assertTrue(client.wasCommError(unsent())); + assertTrue(client.wasRequestUnsent(wrapped(unsent()))); + assertTrue(client.wasCommError(wrapped(unsent()))); + + // Nothing was written because nothing connected. + assertTrue(client.wasRequestUnsent(new ConnectException("Connection refused"))); + assertTrue(client.wasCommError(new ConnectException("Connection refused"))); + + // A comm error, but no proof either way about delivery. + assertFalse(client.wasRequestUnsent(new SocketException("Connection reset"))); + assertTrue(client.wasCommError(new SocketException("Connection reset"))); + assertFalse(client.wasRequestUnsent(new UnknownHostException("nosuchhost"))); + assertTrue(client.wasCommError(new UnknownHostException("nosuchhost"))); + + // A bare IOException may have been sent and applied, so it proves nothing. + assertFalse(client.wasRequestUnsent(new IOException("Broken pipe"))); + assertFalse(client.wasCommError(new IOException("Broken pipe"))); + } + + @Test + public void testHttpJdkSolrClientClassification() throws Exception { + try (HttpJdkSolrClient client = new HttpJdkSolrClient.Builder(DEAD_URL).build()) { + assertSharedHttpClassification(client); + + // The connection was never established, so the request cannot have been written. + assertTrue(client.wasRequestUnsent(new HttpConnectTimeoutException("timed out"))); + assertTrue(client.wasCommError(new HttpConnectTimeoutException("timed out"))); + } + } + + @Test + public void testHttpJettySolrClientClassification() throws Exception { + try (HttpJettySolrClient client = new HttpJettySolrClient.Builder(DEAD_URL).build()) { + assertSharedHttpClassification(client); + + // Jetty's connection-lost types are communication errors, but a connection can end after the + // request was fully written, so they must never claim it was unsent. Whether it was is + // answered at the throw site by the request-commit listener instead. + for (Throwable lost : + new Throwable[] { + new EofException("Connection reset by peer"), new ClosedChannelException() + }) { + assertTrue(lost.getClass().getName(), client.wasCommError(lost)); + assertFalse(lost.getClass().getName(), client.wasRequestUnsent(lost)); + } + } + } + + /** A plain {@link SolrClient} cannot tell, and must never claim otherwise. */ + @Test + public void testDefaultIsAlwaysFalse() { + SolrClient client = + new SolrClient() { + @Override + public NamedList request(SolrRequest request, String collection) { + throw new UnsupportedOperationException(); + } + + @Override + public void close() {} + }; + assertFalse(client.wasRequestUnsent(unsent())); + assertFalse(client.wasCommError(new SocketException("Connection reset"))); + } +} From dece2f3cdc5042c7762b343ba7a728c1d367c774 Mon Sep 17 00:00:00 2001 From: chan-dx Date: Sun, 30 Aug 2026 16:11:58 +0100 Subject: [PATCH 09/20] SOLR-18402: Make the cache refresh path explicit and testable --- .../solr/client/solrj/impl/CloudSolrClientCacheTest.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java index a98001abdad..8f00535d884 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java @@ -407,6 +407,9 @@ private static class RecordingCloudSolrClient extends CloudSolrClient implements private volatile Invocation defaultInvocation; private final List stateHistory = Collections.synchronizedList(new ArrayList<>()); private final NamedList okResponse; + // Answers "cannot tell" to both classification predicates, which these tests do not exercise. + // Stub it if a test needs a communication error. + private final HttpSolrClient httpClient = mock(HttpSolrClient.class); RecordingCloudSolrClient(ClusterStateProvider provider, int refreshThreads) { this(provider, true, true, false, refreshThreads); @@ -475,7 +478,7 @@ public ClusterStateProvider getClusterStateProvider() { @Override public HttpSolrClient getHttpClient() { - return mock(HttpSolrClient.class); + return httpClient; } @FunctionalInterface From 69e3d1dccd3a997cd090567317560159cd8e203c Mon Sep 17 00:00:00 2001 From: chan-dx Date: Sun, 30 Aug 2026 16:11:59 +0100 Subject: [PATCH 10/20] SOLR-18402: Modify mayReplay logic to mayReplayAfterCommError. --- .../client/solrj/impl/CloudSolrClient.java | 15 +++-- .../solrj/impl/CloudSolrClientCacheTest.java | 60 ++++++++++++++++--- 2 files changed, 61 insertions(+), 14 deletions(-) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java index e2ffc530171..31283534cbe 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java @@ -721,9 +721,10 @@ protected NamedList requestWithRetryOnStaleState( : SolrException.ErrorCode.UNKNOWN.code; final boolean wasCommError = wasCommError(exc); - // An update may already have been applied; only replay one the transport proves never - // arrived. - final boolean mayReplay = + // A communication error says nothing about whether an update was applied; only replay one + // the transport proves never arrived. A 503 is the server declining to process it, so that + // path is unaffected. + final boolean mayReplayAfterCommError = request.getRequestType() != SolrRequestType.UPDATE || wasRequestUnsent(exc); if (wasCommError @@ -757,7 +758,7 @@ protected NamedList requestWithRetryOnStaleState( } } // if it is a communication error , we must try again - if (mayReplay && retryCount < MAX_STALE_RETRIES) { + if ((!wasCommError || mayReplayAfterCommError) && retryCount < MAX_STALE_RETRIES) { // may be, we have a stale version of the collection state, // and we could not get any information from the server // it is probably not worth trying again and again because @@ -820,11 +821,13 @@ protected NamedList requestWithRetryOnStaleState( for (DocCollection ext : requestedCollections) { DocCollection latestStateFromZk = getDocCollection(ext.getName(), null); if (latestStateFromZk.getZNodeVersion() != ext.getZNodeVersion()) { - // looks like we couldn't reach the server because the state was stale == retry - stateWasStale = true; // we just pulled state from ZK, so update the cache so that the retry uses it collectionStateCache.put( ext.getName(), new ExpiringCachedDocCollection(latestStateFromZk)); + if (mayReplayAfterCommError) { + // looks like we couldn't reach the server because the state was stale == retry + stateWasStale = true; + } } } } diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java index 8f00535d884..7ed0e522e87 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java @@ -44,6 +44,7 @@ import java.util.function.Function; import java.util.function.Supplier; import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.client.solrj.RemoteSolrException; import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrServerException; import org.apache.solr.client.solrj.jetty.LBJettySolrClient; @@ -146,7 +147,6 @@ public void testUpdateIsNotReplayedWhenItMayHaveBeenApplied() throws Exception { String collName = "gettingstarted"; Set livenodes = new HashSet<>(); Map refs = new HashMap<>(); - Map colls = new HashMap<>(); Map> responses = new HashMap<>(); LBJettySolrClient mockLbclient = getMockLbHttpSolrClient(responses); @@ -158,10 +158,12 @@ public void testUpdateIsNotReplayedWhenItMayHaveBeenApplied() throws Exception { protected LBSolrClient createOrGetLbClient(HttpSolrClient myClient) { return mockLbclient; } - }.build()) { + } + // Pin the routing so the update takes the load-balanced path and the transport's + // failure arrives unwrapped. + .sendUpdatesToAnyReplica().build()) { livenodes.addAll(Set.of("192.168.1.108:7574_solr", "192.168.1.108:8983_solr")); refs.put(collName, new ClusterState.CollectionRef(loadCollection(collName, 1))); - colls.put(collName, loadCollection(collName, 1)); // Not a ConnectException: the transport cannot prove this request never left. responses.put( @@ -172,16 +174,58 @@ protected LBSolrClient createOrGetLbClient(HttpSolrClient myClient) { }); UpdateRequest update = new UpdateRequest().add("id", "123", "desc", "Something 0"); - // The routing randomization decides whether the failure arrives wrapped, so match the cause. - Exception thrown = expectThrows(Exception.class, () -> cloudClient.request(update, collName)); - assertTrue( - "the transport's failure must reach the caller", - SolrException.hasCause(thrown, SocketException.class)); + expectThrows(SocketException.class, () -> cloudClient.request(update, collName)); assertEquals( "an update that may have been applied must not be replayed", 1, lbhttpRequestCount.get()); } } + /** + * A 503 is the server declining to process the update, not a communication failure, so it stays + * retryable. {@link CloudSolrClient#directUpdate} raises this shape when a shard replica is + * unavailable. + */ + public void testUpdateIsRetriedOnRouteExceptionWith503() throws Exception { + String collName = "gettingstarted"; + Set livenodes = new HashSet<>(); + Map refs = new HashMap<>(); + + Map> responses = new HashMap<>(); + NamedList okResponse = new NamedList<>(); + okResponse.add("responseHeader", new NamedList<>(Map.of("status", 0))); + LBJettySolrClient mockLbclient = getMockLbHttpSolrClient(responses); + AtomicInteger lbhttpRequestCount = new AtomicInteger(); + try (ClusterStateProvider clusterStateProvider = getStateProvider(livenodes, refs); + CloudSolrClient cloudClient = + new RandomizingCloudSolrClientBuilder(clusterStateProvider) { + @Override + protected LBSolrClient createOrGetLbClient(HttpSolrClient myClient) { + return mockLbclient; + } + }.sendUpdatesToAnyReplica().build()) { + livenodes.addAll(Set.of("192.168.1.108:7574_solr", "192.168.1.108:8983_solr")); + refs.put(collName, new ClusterState.CollectionRef(loadCollection(collName, 1))); + + NamedList shardFailures = new NamedList<>(); + shardFailures.add( + "http://127.0.0.1:8983/solr/gettingstarted_shard1_replica_n1", + new RemoteSolrException("127.0.0.1:8983", 503, "Service Unavailable", null)); + responses.put( + "request", + o -> { + if (lbhttpRequestCount.incrementAndGet() == 1) { + return new CloudSolrClient.RouteException( + SolrException.ErrorCode.SERVICE_UNAVAILABLE, shardFailures, Map.of()); + } + return okResponse; + }); + + UpdateRequest update = new UpdateRequest().add("id", "123", "desc", "Something 0"); + cloudClient.request(update, collName); + assertEquals("a 503 must still be retried", 2, lbhttpRequestCount.get()); + } + } + public void testStaleStateRetrySkipsStateVersionBeforeWait() throws Exception { String collName = "gettingstarted"; Set liveNodes = new HashSet<>(Set.of("192.168.1.108:8983_solr")); From c8d6decf7d1deab5a7af51c3b5d06ca14dc12785 Mon Sep 17 00:00:00 2001 From: chan-dx Date: Sun, 30 Aug 2026 16:11:59 +0100 Subject: [PATCH 11/20] SOLR-18402: HttpJettySolrClient: report a lost HTTP/2 session as EofException --- .../apache/solr/client/solrj/jetty/HttpJettySolrClient.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java b/solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java index ef724fead1b..57ed0d6f86c 100644 --- a/solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java +++ b/solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java @@ -523,9 +523,11 @@ public NamedList request(SolrRequest solrRequest, String collection) // Jetty HTTP/2 throws IllegalStateException ("session closed") when the connection is lost. abortCause = e; throw committed.get() - ? new SolrServerException("Connection lost at: " + url, new IOException(e)) + ? new SolrServerException( + "Connection lost at: " + url, new EofException("HTTP/2 session closed", e)) : new SolrServerException( - "Connection lost at: " + url, new RequestNotSentException(e.getMessage(), e)); + "Connection failed before the request was sent to: " + url, + new RequestNotSentException(e.getMessage(), e)); } catch (SolrServerException | RuntimeException sse) { abortCause = sse; throw sse; From 33f413033e7b7f362f4143a54e8589c6cfeeb695 Mon Sep 17 00:00:00 2001 From: chan-dx Date: Sun, 30 Aug 2026 16:11:59 +0100 Subject: [PATCH 12/20] SOLR-18402: Update changelog --- .../SOLR-18402-consolidate-retry-classification.yml | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/changelog/unreleased/SOLR-18402-consolidate-retry-classification.yml b/changelog/unreleased/SOLR-18402-consolidate-retry-classification.yml index ec7167ca29b..b095a396737 100644 --- a/changelog/unreleased/SOLR-18402-consolidate-retry-classification.yml +++ b/changelog/unreleased/SOLR-18402-consolidate-retry-classification.yml @@ -1,8 +1,6 @@ title: > - SolrJ: transports now classify their own failures via SolrClient.wasRequestUnsent / - wasCommError; CloudSolrClient and LBSolrClient ask instead of matching exception types. - CloudSolrClient now replays an update only when the transport proves it was never sent, and - LBSolrClient fails over on a bare IOException instead of aborting. + SolrJ transports now classify their own failures via SolrClient.wasRequestUnsent / + wasCommError, and CloudSolrClient replays an update only when the transport proves it unsent type: changed authors: - name: Han Chan From 2081d4d9bd9c68bd7ab3c0c8c10fb43b7d9ee98d Mon Sep 17 00:00:00 2001 From: chan-dx Date: Sun, 30 Aug 2026 16:11:59 +0100 Subject: [PATCH 13/20] SOLR-18402: Verifies that Jetty connection-loss errors are classified as communication failures without falsely claiming the request was unsent, even when wrapped or raised after the request was already committed. --- .../solrj/impl/SolrClientErrorClassificationTest.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/SolrClientErrorClassificationTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/SolrClientErrorClassificationTest.java index d9ea47391ef..7bc5ef83317 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/SolrClientErrorClassificationTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/SolrClientErrorClassificationTest.java @@ -96,10 +96,15 @@ public void testHttpJettySolrClientClassification() throws Exception { // answered at the throw site by the request-commit listener instead. for (Throwable lost : new Throwable[] { - new EofException("Connection reset by peer"), new ClosedChannelException() + new EofException("Connection reset by peer"), + new ClosedChannelException(), + // The shape HttpJettySolrClient raises once an HTTP/2 session is lost after commit. + new EofException("HTTP/2 session closed", new IllegalStateException("session closed")) }) { assertTrue(lost.getClass().getName(), client.wasCommError(lost)); + assertTrue(lost.getClass().getName(), client.wasCommError(wrapped(lost))); assertFalse(lost.getClass().getName(), client.wasRequestUnsent(lost)); + assertFalse(lost.getClass().getName(), client.wasRequestUnsent(wrapped(lost))); } } } From 422b2ff78ba0f1c9ab4d9b226c274f60aea29d09 Mon Sep 17 00:00:00 2001 From: chan-dx Date: Mon, 31 Aug 2026 13:47:56 +0100 Subject: [PATCH 14/20] SOLR-18402: Stop replaying updates on a 503 --- .../client/solrj/impl/CloudSolrClient.java | 12 ++++----- .../solrj/impl/CloudSolrClientCacheTest.java | 26 +++++++++---------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java index 31283534cbe..5ee3c4e863d 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java @@ -721,10 +721,10 @@ protected NamedList requestWithRetryOnStaleState( : SolrException.ErrorCode.UNKNOWN.code; final boolean wasCommError = wasCommError(exc); - // A communication error says nothing about whether an update was applied; only replay one - // the transport proves never arrived. A 503 is the server declining to process it, so that - // path is unaffected. - final boolean mayReplayAfterCommError = + // Neither a comm error nor a 503 proves an update went unapplied: directUpdate raises + // RouteException only after collecting every shard's result. Replay only what the transport + // proves never arrived. + final boolean mayReplay = request.getRequestType() != SolrRequestType.UPDATE || wasRequestUnsent(exc); if (wasCommError @@ -758,7 +758,7 @@ protected NamedList requestWithRetryOnStaleState( } } // if it is a communication error , we must try again - if ((!wasCommError || mayReplayAfterCommError) && retryCount < MAX_STALE_RETRIES) { + if (mayReplay && retryCount < MAX_STALE_RETRIES) { // may be, we have a stale version of the collection state, // and we could not get any information from the server // it is probably not worth trying again and again because @@ -824,7 +824,7 @@ protected NamedList requestWithRetryOnStaleState( // we just pulled state from ZK, so update the cache so that the retry uses it collectionStateCache.put( ext.getName(), new ExpiringCachedDocCollection(latestStateFromZk)); - if (mayReplayAfterCommError) { + if (mayReplay) { // looks like we couldn't reach the server because the state was stale == retry stateWasStale = true; } diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java index 7ed0e522e87..84f3a7f7d7e 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java @@ -181,18 +181,16 @@ protected LBSolrClient createOrGetLbClient(HttpSolrClient myClient) { } /** - * A 503 is the server declining to process the update, not a communication failure, so it stays - * retryable. {@link CloudSolrClient#directUpdate} raises this shape when a shard replica is - * unavailable. + * {@link CloudSolrClient#directUpdate} raises a {@link CloudSolrClient.RouteException} only after + * collecting every shard's result, so a 503 from one shard can follow success on another and a + * replay would re-apply those. */ - public void testUpdateIsRetriedOnRouteExceptionWith503() throws Exception { + public void testUpdateIsNotRetriedOnRouteExceptionWith503() throws Exception { String collName = "gettingstarted"; Set livenodes = new HashSet<>(); Map refs = new HashMap<>(); Map> responses = new HashMap<>(); - NamedList okResponse = new NamedList<>(); - okResponse.add("responseHeader", new NamedList<>(Map.of("status", 0))); LBJettySolrClient mockLbclient = getMockLbHttpSolrClient(responses); AtomicInteger lbhttpRequestCount = new AtomicInteger(); try (ClusterStateProvider clusterStateProvider = getStateProvider(livenodes, refs); @@ -213,16 +211,18 @@ protected LBSolrClient createOrGetLbClient(HttpSolrClient myClient) { responses.put( "request", o -> { - if (lbhttpRequestCount.incrementAndGet() == 1) { - return new CloudSolrClient.RouteException( - SolrException.ErrorCode.SERVICE_UNAVAILABLE, shardFailures, Map.of()); - } - return okResponse; + lbhttpRequestCount.incrementAndGet(); + return new CloudSolrClient.RouteException( + SolrException.ErrorCode.SERVICE_UNAVAILABLE, shardFailures, Map.of()); }); UpdateRequest update = new UpdateRequest().add("id", "123", "desc", "Something 0"); - cloudClient.request(update, collName); - assertEquals("a 503 must still be retried", 2, lbhttpRequestCount.get()); + expectThrows( + CloudSolrClient.RouteException.class, () -> cloudClient.request(update, collName)); + assertEquals( + "a 503 may follow partial success, so it must not be replayed", + 1, + lbhttpRequestCount.get()); } } From 75c5fed97a1d958bf215816b4067194e120c1afa Mon Sep 17 00:00:00 2001 From: chan-dx Date: Mon, 31 Aug 2026 15:22:16 +0100 Subject: [PATCH 15/20] SOLR-18402: Update upgrade note --- .../upgrade-notes/pages/major-changes-in-solr-10.adoc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-10.adoc b/solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-10.adoc index cfa96a93865..c782c326ac1 100644 --- a/solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-10.adoc +++ b/solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-10.adoc @@ -103,6 +103,12 @@ Its builder will dynamically detect if solr-jetty is available and use that, oth CommonParams.QT has been un-deprecated. Nonetheless, if your code makes explicit reference to "qt" when constructing a standard request, there is usually a better way. +`CloudSolrClient` now retries a failed update only when the transport can prove the request never reached the server. +Previously any communication error, or a 503, caused a retry, which could re-send an update that had already been partially applied. + +`SolrClient` gains `wasRequestUnsent(Throwable)` and `wasCommError(Throwable)`, both defaulting to `false` and overridden per transport. +`CloudSolrClient.wasCommError` is now `public`, and `LBSolrClient.isConnectException` has been removed; override `wasRequestUnsent` on the transport client instead. + === Jetty Configuration Solr 10.1 upgrades the server to Eclipse Jetty 12.1, which removed Jetty's directory-scanning deployer (the `DeploymentManager` and `ContextProvider` classes). From a5181a13b5f066646e325e8a4693d9aeed5a4b49 Mon Sep 17 00:00:00 2001 From: chan-dx Date: Sat, 5 Sep 2026 17:21:49 +0100 Subject: [PATCH 16/20] SOLR-18402: Revert the ref-guide upgrade note --- .../upgrade-notes/pages/major-changes-in-solr-10.adoc | 6 ------ 1 file changed, 6 deletions(-) diff --git a/solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-10.adoc b/solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-10.adoc index c782c326ac1..cfa96a93865 100644 --- a/solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-10.adoc +++ b/solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-10.adoc @@ -103,12 +103,6 @@ Its builder will dynamically detect if solr-jetty is available and use that, oth CommonParams.QT has been un-deprecated. Nonetheless, if your code makes explicit reference to "qt" when constructing a standard request, there is usually a better way. -`CloudSolrClient` now retries a failed update only when the transport can prove the request never reached the server. -Previously any communication error, or a 503, caused a retry, which could re-send an update that had already been partially applied. - -`SolrClient` gains `wasRequestUnsent(Throwable)` and `wasCommError(Throwable)`, both defaulting to `false` and overridden per transport. -`CloudSolrClient.wasCommError` is now `public`, and `LBSolrClient.isConnectException` has been removed; override `wasRequestUnsent` on the transport client instead. - === Jetty Configuration Solr 10.1 upgrades the server to Eclipse Jetty 12.1, which removed Jetty's directory-scanning deployer (the `DeploymentManager` and `ContextProvider` classes). From 06f541a82c5ede8c31027f65831fd2c23034b513 Mon Sep 17 00:00:00 2001 From: chan-dx Date: Sat, 5 Sep 2026 17:34:19 +0100 Subject: [PATCH 17/20] SOLR-18402: Move failure classification to HttpSolrClient Per review, these are HTTP transport concerns, not something every SolrClient can answer. LBSolrClient.getClient now declares HttpSolrClient, which every implementation already returned. --- .../apache/solr/client/solrj/SolrClient.java | 17 ------ .../client/solrj/impl/CloudSolrClient.java | 16 +----- .../client/solrj/impl/HttpSolrClient.java | 11 +++- .../solr/client/solrj/impl/LBSolrClient.java | 8 ++- .../impl/LBSolrClientRetryUnsentTest.java | 55 +++++++++---------- .../SolrClientErrorClassificationTest.java | 27 ++------- 6 files changed, 49 insertions(+), 85 deletions(-) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/SolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/SolrClient.java index 246f7b45e78..5d54d12155a 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/SolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/SolrClient.java @@ -1194,23 +1194,6 @@ public final NamedList request(final SolrRequest request) return request(request, null); } - /** - * Whether the failure proves the request never reached the server, making a replay safe even when - * the request isn't idempotent. Only the transport can answer this; the default is {@code false}, - * meaning "cannot tell" rather than "the request was sent". - */ - public boolean wasRequestUnsent(Throwable t) { - return false; - } - - /** - * Whether this is a transport-level communication failure rather than a response from the server. - * Implementations must keep {@link #wasRequestUnsent} a subset of this. - */ - public boolean wasCommError(Throwable t) { - return false; - } - /** * This method defines the context in which this Solr client is being used (e.g. for internal * communication between Solr nodes or as an external client). The default value is {@code diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java index 5ee3c4e863d..2de6c90061c 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java @@ -203,17 +203,6 @@ public ClusterState getClusterState() { return getClusterStateProvider().getClusterState(); } - /** Is this a communication error? We will retry if so. Answered by the underlying transport. */ - @Override - public boolean wasCommError(Throwable t) { - return getHttpClient().wasCommError(t); - } - - @Override - public boolean wasRequestUnsent(Throwable t) { - return getHttpClient().wasRequestUnsent(t); - } - @Override public void close() { closed = true; @@ -720,12 +709,13 @@ protected NamedList requestWithRetryOnStaleState( ? ((SolrException) rootCause).code() : SolrException.ErrorCode.UNKNOWN.code; - final boolean wasCommError = wasCommError(exc); + final boolean wasCommError = getHttpClient().wasCommError(exc); // Neither a comm error nor a 503 proves an update went unapplied: directUpdate raises // RouteException only after collecting every shard's result. Replay only what the transport // proves never arrived. final boolean mayReplay = - request.getRequestType() != SolrRequestType.UPDATE || wasRequestUnsent(exc); + request.getRequestType() != SolrRequestType.UPDATE + || getHttpClient().wasRequestUnsent(exc); if (wasCommError || (exc instanceof RouteException diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java index 59beb013abc..de06e2567dd 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java @@ -362,13 +362,20 @@ public Set getUrlParamNames() { return urlParamNames; } - @Override + /** + * Whether the failure proves the request never reached the server, making a replay safe even when + * the request isn't idempotent. Only the transport can answer this; {@code false} means "cannot + * tell" rather than "the request was sent". + */ public boolean wasRequestUnsent(Throwable t) { return SolrException.hasCause(t, RequestNotSentException.class) || SolrException.hasCause(t, ConnectException.class); } - @Override + /** + * Whether this is a transport-level communication failure rather than a response from the server. + * Subclasses must keep {@link #wasRequestUnsent} a subset of this. + */ public boolean wasCommError(Throwable t) { return SolrException.hasCause(t, SocketException.class) || SolrException.hasCause(t, UnknownHostException.class) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java index 526fd47a676..53f35a92af6 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java @@ -207,7 +207,7 @@ public C getSolrClient() { public LBSolrClient build() { return new LBSolrClient(this) { @Override - protected SolrClient getClient(Endpoint endpoint) { + protected HttpSolrClient getClient(Endpoint endpoint) { return solrClient; } }; @@ -690,7 +690,11 @@ protected Exception doRequest( return ex; } - protected abstract SolrClient getClient(Endpoint endpoint); + /** + * The transport used to reach {@code endpoint}. Declared as an {@link HttpSolrClient} so callers + * can ask it to classify its own failures; {@link Builder} already requires one. + */ + protected abstract HttpSolrClient getClient(Endpoint endpoint); private void startAliveCheckExecutor() { // double-checked locking, but it's OK because we don't *do* anything with aliveCheckExecutor diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBSolrClientRetryUnsentTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBSolrClientRetryUnsentTest.java index 572b0e8bd0d..b7645f9e838 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBSolrClientRetryUnsentTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBSolrClientRetryUnsentTest.java @@ -21,13 +21,12 @@ import java.util.List; import org.apache.solr.SolrTestCase; import org.apache.solr.client.solrj.RequestNotSentException; -import org.apache.solr.client.solrj.SolrClient; import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrRequest.SolrRequestType; import org.apache.solr.client.solrj.SolrServerException; import org.apache.solr.client.solrj.request.QueryRequest; import org.apache.solr.client.solrj.request.UpdateRequest; -import org.apache.solr.common.SolrException; +import org.apache.solr.common.util.IOUtils; import org.apache.solr.common.util.NamedList; import org.junit.Test; @@ -46,38 +45,38 @@ public class LBSolrClientRetryUnsentTest extends SolrTestCase { /** Fails whatever endpoint is tried first with {@code failure}; any later endpoint succeeds. */ private static class FailFirstEndpoint extends LBSolrClient { final List attempted = new ArrayList<>(); - private final Exception failure; + private final HttpSolrClient transport; FailFirstEndpoint(Exception failure) { super(List.of(DEAD_HOST_1, DEAD_HOST_2)); - this.failure = failure; + // A real transport, so the LB asks its real classification rather than a stand-in. + this.transport = + new HttpJdkSolrClient(DEAD_HOST_1.getBaseUrl(), new HttpJdkSolrClient.Builder()) { + @Override + public NamedList requestWithBaseUrl( + String baseUrl, SolrRequest solrRequest, String collection) + throws SolrServerException, IOException { + attempted.add(baseUrl); + if (attempted.size() > 1) { + return new NamedList<>(); + } + if (failure instanceof SolrServerException sse) { + throw sse; + } + throw (IOException) failure; + } + }; } @Override - protected SolrClient getClient(Endpoint endpoint) { - return new SolrClient() { - // Stands in for a transport; the LB asks it rather than inspecting the exception itself. - @Override - public boolean wasRequestUnsent(Throwable t) { - return SolrException.hasCause(t, RequestNotSentException.class); - } - - @Override - public NamedList request(SolrRequest request, String collection) - throws SolrServerException, IOException { - attempted.add(endpoint.getBaseUrl()); - if (attempted.size() > 1) { - return new NamedList<>(); - } - if (failure instanceof SolrServerException sse) { - throw sse; - } - throw (IOException) failure; - } - - @Override - public void close() {} - }; + protected HttpSolrClient getClient(Endpoint endpoint) { + return transport; + } + + @Override + public void close() { + super.close(); + IOUtils.closeQuietly(transport); } } diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/SolrClientErrorClassificationTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/SolrClientErrorClassificationTest.java index 7bc5ef83317..06479b174d7 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/SolrClientErrorClassificationTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/SolrClientErrorClassificationTest.java @@ -24,18 +24,16 @@ import java.nio.channels.ClosedChannelException; import org.apache.solr.SolrTestCase; import org.apache.solr.client.solrj.RequestNotSentException; -import org.apache.solr.client.solrj.SolrClient; -import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrServerException; import org.apache.solr.client.solrj.jetty.HttpJettySolrClient; -import org.apache.solr.common.util.NamedList; import org.eclipse.jetty.io.EofException; import org.junit.Test; /** - * {@link SolrClient#wasRequestUnsent} and {@link SolrClient#wasCommError} are pure functions of the - * failure, so each transport's answers can be asserted directly rather than raced for through an - * integration test. No server is needed; the clients are never asked to send anything. + * {@link HttpSolrClient#wasRequestUnsent} and {@link HttpSolrClient#wasCommError} are pure + * functions of the failure, so each transport's answers can be asserted directly rather than raced + * for through an integration test. No server is needed; the clients are never asked to send + * anything. * *

The negative cases matter most: {@code wasRequestUnsent} returning false means "cannot tell", * and treating a failure as unsent when it isn't would replay a non-idempotent update. @@ -108,21 +106,4 @@ public void testHttpJettySolrClientClassification() throws Exception { } } } - - /** A plain {@link SolrClient} cannot tell, and must never claim otherwise. */ - @Test - public void testDefaultIsAlwaysFalse() { - SolrClient client = - new SolrClient() { - @Override - public NamedList request(SolrRequest request, String collection) { - throw new UnsupportedOperationException(); - } - - @Override - public void close() {} - }; - assertFalse(client.wasRequestUnsent(unsent())); - assertFalse(client.wasCommError(new SocketException("Connection reset"))); - } } From f40654ef866d26296656f9c0e128749f9ce06469 Mon Sep 17 00:00:00 2001 From: chan-dx Date: Sat, 5 Sep 2026 18:06:09 +0100 Subject: [PATCH 18/20] SOLR-18402: Collapse the LB transport catch blocks into one Per review, four near-identical blocks become a single catch delegating to a shared mayFailOver, used by both the sync and async paths. --- .../client/solrj/impl/LBAsyncSolrClient.java | 31 +------------ .../solr/client/solrj/impl/LBSolrClient.java | 45 +++++++------------ .../impl/LBSolrClientRetryUnsentTest.java | 16 +++++++ 3 files changed, 34 insertions(+), 58 deletions(-) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBAsyncSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBAsyncSolrClient.java index 5d9d59ee0f0..ee0282f2584 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBAsyncSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBAsyncSolrClient.java @@ -17,11 +17,8 @@ package org.apache.solr.client.solrj.impl; import java.io.IOException; -import java.net.SocketException; -import java.net.SocketTimeoutException; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; -import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicReference; import org.apache.solr.client.solrj.RemoteSolrException; import org.apache.solr.client.solrj.SolrClient; @@ -200,32 +197,8 @@ private void onFailedRequest( } listener.onFailure(e, false); } - } catch (SocketException e) { - if (!isNonRetryable || getClient(endpoint).wasRequestUnsent(e)) { - listener.onFailure((!isZombie) ? makeServerAZombie(endpoint, e) : e, true); - } else { - listener.onFailure(e, false); - } - } catch (SocketTimeoutException e) { - if (!isNonRetryable) { - listener.onFailure((!isZombie) ? makeServerAZombie(endpoint, e) : e, true); - } else { - listener.onFailure(e, false); - } - } catch (SolrServerException e) { - Throwable rootCause = e.getRootCause(); - if (!isNonRetryable - && (rootCause instanceof IOException || rootCause instanceof TimeoutException)) { - listener.onFailure((!isZombie) ? makeServerAZombie(endpoint, e) : e, true); - } else if (isNonRetryable && getClient(endpoint).wasRequestUnsent(e)) { - // Nothing of the request reached the server, so replaying it elsewhere is safe even though - // it isn't idempotent. - listener.onFailure((!isZombie) ? makeServerAZombie(endpoint, e) : e, true); - } else { - listener.onFailure(e, false); - } - } catch (IOException e) { - if (!isNonRetryable || getClient(endpoint).wasRequestUnsent(e)) { + } catch (SolrServerException | IOException e) { + if (mayFailOver(endpoint, e, isNonRetryable)) { listener.onFailure((!isZombie) ? makeServerAZombie(endpoint, e) : e, true); } else { listener.onFailure(e, false); diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java index 53f35a92af6..730042573bd 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java @@ -20,8 +20,6 @@ import java.io.IOException; import java.lang.invoke.MethodHandles; import java.lang.ref.WeakReference; -import java.net.SocketException; -import java.net.SocketTimeoutException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -652,33 +650,8 @@ protected Exception doRequest( } throw e; } - } catch (SocketException e) { - if (!isNonRetryable || getClient(baseUrl).wasRequestUnsent(e)) { - ex = (!isZombie) ? makeServerAZombie(baseUrl, e) : e; - } else { - throw e; - } - } catch (SocketTimeoutException e) { - if (!isNonRetryable) { - ex = (!isZombie) ? makeServerAZombie(baseUrl, e) : e; - } else { - throw e; - } - } catch (SolrServerException e) { - Throwable rootCause = e.getRootCause(); - if (!isNonRetryable - && (rootCause instanceof IOException || rootCause instanceof TimeoutException)) { - ex = (!isZombie) ? makeServerAZombie(baseUrl, e) : e; - } else if (isNonRetryable && getClient(baseUrl).wasRequestUnsent(e)) { - // Nothing of the request reached the server, so replaying it elsewhere is safe even though - // it isn't idempotent. - ex = (!isZombie) ? makeServerAZombie(baseUrl, e) : e; - } else { - throw e; - } - } catch (IOException e) { - // A transport may throw one directly rather than wrapping it in a SolrServerException. - if (!isNonRetryable || getClient(baseUrl).wasRequestUnsent(e)) { + } catch (SolrServerException | IOException e) { + if (mayFailOver(baseUrl, e, isNonRetryable)) { ex = (!isZombie) ? makeServerAZombie(baseUrl, e) : e; } else { throw e; @@ -690,6 +663,20 @@ protected Exception doRequest( return ex; } + /** + * Whether {@code e} permits trying the next endpoint. A request that isn't safe to replay fails + * over only when the transport proves nothing was sent; anything else fails over on any network + * failure. + */ + protected boolean mayFailOver(Endpoint endpoint, Exception e, boolean isNonRetryable) { + if (getClient(endpoint).wasRequestUnsent(e)) { + return true; + } + Throwable rootCause = (e instanceof SolrServerException sse) ? sse.getRootCause() : e; + return !isNonRetryable + && (rootCause instanceof IOException || rootCause instanceof TimeoutException); + } + /** * The transport used to reach {@code endpoint}. Declared as an {@link HttpSolrClient} so callers * can ask it to classify its own failures; {@link Builder} already requires one. diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBSolrClientRetryUnsentTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBSolrClientRetryUnsentTest.java index b7645f9e838..c4f057b56e5 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBSolrClientRetryUnsentTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBSolrClientRetryUnsentTest.java @@ -148,6 +148,22 @@ public void testUpdateIsNotRetriedOnBareIOException() { } } + /** + * A query must fail over whenever the transport proves the request unsent, even if the deepest + * cause isn't an {@link IOException}. + */ + @Test + public void testQueryIsRetriedWhenUnsentButRootCauseIsNotIO() throws Exception { + IllegalStateException sessionClosed = new IllegalStateException("session closed"); + SolrServerException failure = + new SolrServerException( + "Connection failed before the request was sent to: " + DEAD_HOST_1.getUrl(), + new RequestNotSentException(sessionClosed.getMessage(), sessionClosed)); + assertEquals( + List.of(DEAD_HOST_1.getBaseUrl(), DEAD_HOST_2.getBaseUrl()), + requestReturningAttemptedUrls(failure, new QueryRequest())); + } + /** * Parity with LBAsyncSolrClient, which already retried a bare {@link RequestNotSentException}. */ From 27896b5074eee9549323e71eb71f84ba9117f90a Mon Sep 17 00:00:00 2001 From: chan-dx Date: Sat, 5 Sep 2026 18:11:38 +0100 Subject: [PATCH 19/20] SOLR-18402: Update changelog for the moved predicates --- .../unreleased/SOLR-18402-consolidate-retry-classification.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/unreleased/SOLR-18402-consolidate-retry-classification.yml b/changelog/unreleased/SOLR-18402-consolidate-retry-classification.yml index b095a396737..39529c2dd99 100644 --- a/changelog/unreleased/SOLR-18402-consolidate-retry-classification.yml +++ b/changelog/unreleased/SOLR-18402-consolidate-retry-classification.yml @@ -1,5 +1,5 @@ title: > - SolrJ transports now classify their own failures via SolrClient.wasRequestUnsent / + SolrJ transports now classify their own failures via HttpSolrClient.wasRequestUnsent / wasCommError, and CloudSolrClient replays an update only when the transport proves it unsent type: changed authors: From 84ebe75e482516bfda9ef4a39fa09340e79086fe Mon Sep 17 00:00:00 2001 From: chan-dx Date: Sun, 6 Sep 2026 14:17:02 +0100 Subject: [PATCH 20/20] SOLR-18402: Drop the unreachable non-HTTP branch in doRequest. With getClient declared HttpSolrClient the instanceof was always true, so the helper now calls requestWithBaseUrl directly and its stale SOLR-17541 TODO goes with it. --- .../solr/client/solrj/impl/LBSolrClient.java | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java index 730042573bd..f421f4a8bec 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java @@ -609,20 +609,11 @@ private NamedList doRequest(Endpoint endpoint, SolrRequest solrReques return doRequest(solrClient, endpoint.getBaseUrl(), endpoint.getCore(), solrRequest); } - // TODO SOLR-17541 should remove the need for the special-casing below; remove as a part of that - // ticket. + // getClient(...) may return a client that isn't pointed at the desired URL, or at any URL at all. private NamedList doRequest( - SolrClient solrClient, String baseUrl, String collection, SolrRequest solrRequest) + HttpSolrClient solrClient, String baseUrl, String collection, SolrRequest solrRequest) throws SolrServerException, IOException { - // Some implementations of LBSolrClient.getClient(...) return a HttpSolrClient that may not - // be pointed at the desired URL (or any URL for that matter). We special-case that here to - // ensure the appropriate URL is provided. - if (solrClient instanceof HttpSolrClient hasReqWithUrl) { - return hasReqWithUrl.requestWithBaseUrl(baseUrl, solrRequest, collection); - } - - // Assume provided client already uses 'baseUrl' - return solrClient.request(solrRequest, collection); + return solrClient.requestWithBaseUrl(baseUrl, solrRequest, collection); } protected Exception doRequest(