This is an automated email from the ASF dual-hosted git repository. dsmiley pushed a commit to branch branch_10x in repository https://gitbox.apache.org/repos/asf/solr.git
commit 0b7ace7f37ff54bbf039b7395bca2ba2a79969eb Author: David Smiley <[email protected]> AuthorDate: Wed Aug 26 09:28:21 2026 -0400 SOLR-18401: HttpJettySolrClient: detect if request wasn't sent; retry (#4805) HttpJettySolrClient: detect if request wasn't sent; throw a new RequestNotSentException. CloudSolrClient & LBSolrClient will detect it and retry -- typically on another replica. (cherry picked from commit a98481d92279942a19148e5bf27bd5e6d15349a5) --- .../unreleased/SOLR-18401-retry-unsent-request.yml | 9 ++ .../solr/cloud/TestMiniSolrCloudClusterSSL.java | 5 +- .../client/solrj/jetty/HttpJettySolrClient.java | 34 +++++- .../solr/client/solrj/RequestNotSentException.java | 36 ++++++ .../solr/client/solrj/impl/CloudSolrClient.java | 12 +- .../solr/client/solrj/impl/LBAsyncSolrClient.java | 9 +- .../solr/client/solrj/impl/LBSolrClient.java | 7 +- .../java/org/apache/solr/common/SolrException.java | 20 ++++ .../solrj/impl/LBSolrClientRetryUnsentTest.java | 122 +++++++++++++++++++++ 9 files changed, 242 insertions(+), 12 deletions(-) diff --git a/changelog/unreleased/SOLR-18401-retry-unsent-request.yml b/changelog/unreleased/SOLR-18401-retry-unsent-request.yml new file mode 100644 index 00000000000..919810e11bc --- /dev/null +++ b/changelog/unreleased/SOLR-18401-retry-unsent-request.yml @@ -0,0 +1,9 @@ +title: > + HttpJettySolrClient: detect if request wasn't sent; means retry-able. + CloudSolrClient & LBSolrClient will detect it. +type: changed +authors: + - name: David Smiley +links: + - name: SOLR-18401 + url: https://issues.apache.org/jira/browse/SOLR-18401 diff --git a/solr/core/src/test/org/apache/solr/cloud/TestMiniSolrCloudClusterSSL.java b/solr/core/src/test/org/apache/solr/cloud/TestMiniSolrCloudClusterSSL.java index 8fbbe214fe0..39d6561381c 100644 --- a/solr/core/src/test/org/apache/solr/cloud/TestMiniSolrCloudClusterSSL.java +++ b/solr/core/src/test/org/apache/solr/cloud/TestMiniSolrCloudClusterSSL.java @@ -31,6 +31,7 @@ import org.apache.solr.client.solrj.impl.SolrHttpConstants; import org.apache.solr.client.solrj.jetty.HttpJettySolrClient; import org.apache.solr.client.solrj.request.CollectionAdminRequest; import org.apache.solr.client.solrj.request.CoreAdminRequest; +import org.apache.solr.common.SolrException; import org.apache.solr.common.params.CoreAdminParams.CoreAdminAction; import org.apache.solr.embedded.JettyConfig; import org.apache.solr.embedded.JettySolrRunner; @@ -205,8 +206,8 @@ public class TestMiniSolrCloudClusterSSL extends SolrTestCaseJ4 { } }); assertTrue( - "Expected an root cause SSL Exception, got: " + ex.toString(), - ex.getCause() instanceof SSLException); + "Expected an SSL Exception in the cause chain, got: " + ex, + SolrException.hasCause(ex, SSLException.class)); } } finally { cluster.shutdown(); 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 e561dc4d056..77e1f7e3f1a 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 @@ -36,7 +36,9 @@ import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.Semaphore; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicBoolean; import org.apache.solr.client.api.util.SolrVersion; +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; @@ -473,9 +475,26 @@ public class HttpJettySolrClient extends HttpSolrClient { String url = getRequestUrl(solrRequest, collection); Throwable abortCause = null; Request req = null; + // Jetty notifies "commit" once the request headers have been written to the network. Until then + // nothing of the request has reached the server, so a failure is safe to retry elsewhere. + AtomicBoolean committed = new AtomicBoolean(); try { InputStreamResponseListener listener = new InputStreamReleaseTrackingResponseListener(); - req = sendRequest(makeRequest(solrRequest, url, false), listener); + MakeRequestReturnValue mrrv = makeRequest(solrRequest, url, false); + mrrv.request.onRequestCommit(r -> committed.set(true)); + try { + req = sendRequest(mrrv, listener); + } catch (IOException e) { + // Writing the body can fail on this thread rather than asynchronously, typically when the + // pooled connection was already closed. + abortCause = e; + req = mrrv.request; + throw committed.get() + ? new SolrServerException("IOException occurred when talking to server at: " + url, e) + : new SolrServerException( + "Connection failed before the request was sent to: " + url, + new RequestNotSentException(e.getMessage(), e)); + } // only waits for headers, so use the idle timeout Response response = listener.get(idleTimeoutMillis, TimeUnit.MILLISECONDS); url = req.getURI().toString(); @@ -497,14 +516,21 @@ public class HttpJettySolrClient extends HttpSolrClient { if (cause instanceof SolrServerException) { throw (SolrServerException) cause; } else if (cause instanceof IOException) { - throw new SolrServerException( - "IOException occurred when talking to server at: " + url, cause); + throw committed.get() + ? new SolrServerException( + "IOException occurred when talking to server at: " + url, cause) + : new SolrServerException( + "Connection failed before the request was sent to: " + url, + new RequestNotSentException(cause.getMessage(), cause)); } throw new SolrServerException(cause.getMessage(), cause); } catch (IllegalStateException e) { // Jetty HTTP/2 throws IllegalStateException ("session closed") when the connection is lost. abortCause = e; - throw new SolrServerException("Connection lost at: " + url, new IOException(e)); + throw committed.get() + ? new SolrServerException("Connection lost at: " + url, new IOException(e)) + : new SolrServerException( + "Connection lost at: " + url, new RequestNotSentException(e.getMessage(), e)); } catch (SolrServerException | RuntimeException sse) { abortCause = sse; throw sse; diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/RequestNotSentException.java b/solr/solrj/src/java/org/apache/solr/client/solrj/RequestNotSentException.java new file mode 100644 index 00000000000..566dd691a5d --- /dev/null +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/RequestNotSentException.java @@ -0,0 +1,36 @@ +/* + * 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; + +import java.io.IOException; +import java.io.Serial; + +/** + * Indicates that a request failed before any of it was written to the network, so the server cannot + * have processed it. Retrying such a request on another node is safe even when it is not + * idempotent. + * + * <p>Typically a pooled connection that the server had already closed. + */ +public class RequestNotSentException extends IOException { + + @Serial private static final long serialVersionUID = 1L; + + public RequestNotSentException(String message, Throwable cause) { + super(message, cause); + } +} 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 18b6fe01f98..376ee50ce60 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 @@ -48,6 +48,7 @@ import java.util.concurrent.locks.Lock; 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; @@ -205,9 +206,14 @@ public abstract class CloudSolrClient extends SolrClient { return getClusterStateProvider().getClusterState(); } - /** Is this a communication error? We will retry if so. */ + /** + * 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) { - return t instanceof SocketException || t instanceof UnknownHostException; + return SolrException.hasCause(t, SocketException.class) + || SolrException.hasCause(t, UnknownHostException.class) + || SolrException.hasCause(t, RequestNotSentException.class); } @Override @@ -712,7 +718,7 @@ public abstract class CloudSolrClient extends SolrClient { ? ((SolrException) rootCause).code() : SolrException.ErrorCode.UNKNOWN.code; - final boolean wasCommError = wasCommError(rootCause); + final boolean wasCommError = wasCommError(exc); if (wasCommError || (exc instanceof RouteException 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 5a91c0314ba..88657cbca14 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 @@ -25,6 +25,7 @@ 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.RequestNotSentException; import org.apache.solr.client.solrj.SolrClient; import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrRequest.SolrRequestType; @@ -218,13 +219,17 @@ public abstract class LBAsyncSolrClient extends LBSolrClient { if (!isNonRetryable && (rootCause instanceof IOException || rootCause instanceof TimeoutException)) { listener.onFailure((!isZombie) ? makeServerAZombie(endpoint, e) : e, true); - } else if (isNonRetryable && isConnectException(rootCause)) { + } else if (isNonRetryable + && (isConnectException(rootCause) + || SolrException.hasCause(e, RequestNotSentException.class))) { + // 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 || isConnectException(e)) { + if (!isNonRetryable || isConnectException(e) || e instanceof RequestNotSentException) { 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 eefbd08b2b1..4a9d63cd375 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 @@ -43,6 +43,7 @@ import java.util.concurrent.TimeoutException; 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; @@ -671,7 +672,11 @@ public abstract class LBSolrClient extends SolrClient { if (!isNonRetryable && (rootCause instanceof IOException || rootCause instanceof TimeoutException)) { ex = (!isZombie) ? makeServerAZombie(baseUrl, e) : e; - } else if (isNonRetryable && isConnectException(rootCause)) { + } else if (isNonRetryable + && (isConnectException(rootCause) + || SolrException.hasCause(e, RequestNotSentException.class))) { + // 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; diff --git a/solr/solrj/src/java/org/apache/solr/common/SolrException.java b/solr/solrj/src/java/org/apache/solr/common/SolrException.java index b8eceee17af..fd9f43f9bc4 100644 --- a/solr/solrj/src/java/org/apache/solr/common/SolrException.java +++ b/solr/solrj/src/java/org/apache/solr/common/SolrException.java @@ -154,6 +154,26 @@ public class SolrException extends RuntimeException { return t; } + /** Cause chains are shallow in practice; the cap only guards against a cyclic chain. */ + private static final int MAX_CAUSE_DEPTH = 100; + + /** + * Whether {@code t} or anything in its cause chain is of the given type. Prefer this to {@link + * #getRootCause} when classifying a failure, since a transport may report it wrapped at any + * depth. + */ + public static boolean hasCause(Throwable t, Class<? extends Throwable> type) { + int depth = 0; + for (Throwable cause = t; + cause != null && depth++ < MAX_CAUSE_DEPTH; + cause = cause.getCause()) { + if (type.isInstance(cause)) { + return true; + } + } + return false; + } + /** * Ensure that the provided tragic exception is wrapped in a 5xx SolrException * 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 new file mode 100644 index 00000000000..3163616e272 --- /dev/null +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBSolrClientRetryUnsentTest.java @@ -0,0 +1,122 @@ +/* + * 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.util.ArrayList; +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.util.NamedList; +import org.junit.Test; + +/** + * A failure that proves the request never reached the server is safe to replay even when + * LBSolrClient would otherwise refuse to retry the request. {@link RequestNotSentException} is that + * proof. + */ +public class LBSolrClientRetryUnsentTest extends SolrTestCase { + + private static final LBSolrClient.Endpoint DEAD_HOST_1 = + new LBSolrClient.Endpoint("http://127.0.0.1:1/solr";); + private static final LBSolrClient.Endpoint DEAD_HOST_2 = + new LBSolrClient.Endpoint("http://127.0.0.1:2/solr";); + + /** Fails whatever endpoint is tried first with {@code failure}; any later endpoint succeeds. */ + private static class FailFirstEndpoint extends LBSolrClient { + final List<String> attempted = new ArrayList<>(); + private final Exception failure; + + FailFirstEndpoint(Exception failure) { + super(List.of(DEAD_HOST_1, DEAD_HOST_2)); + this.failure = failure; + } + + @Override + protected SolrClient getClient(Endpoint endpoint) { + return new SolrClient() { + @Override + public NamedList<Object> 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() {} + }; + } + } + + private static SolrServerException unsentException() { + IOException onTheWire = new IOException("Broken pipe"); + return new SolrServerException( + "Connection failed before the request was sent to: " + DEAD_HOST_1.getUrl(), + new RequestNotSentException(onTheWire.getMessage(), onTheWire)); + } + + private static SolrServerException maybeSentException() { + return new SolrServerException( + "IOException occurred when talking to server at: " + DEAD_HOST_1.getUrl(), + new IOException("Broken pipe")); + } + + private static List<String> requestReturningAttemptedUrls( + Exception failure, SolrRequest<?> request) throws Exception { + try (FailFirstEndpoint client = new FailFirstEndpoint(failure)) { + client.request(new LBSolrClient.Req(request, List.of(DEAD_HOST_1, DEAD_HOST_2))); + return client.attempted; + } + } + + @Test + public void testUpdateIsRetriedWhenRequestWasNeverSent() throws Exception { + assertEquals( + List.of(DEAD_HOST_1.getBaseUrl(), DEAD_HOST_2.getBaseUrl()), + requestReturningAttemptedUrls(unsentException(), new UpdateRequest().add("id", "1"))); + } + + /** LBSolrClient classifies {@link SolrRequestType#UPDATE} as non-retryable. */ + @Test + public void testRequestThatMayHaveBeenReceivedIsNotRetried() { + LBSolrClient.Req req = + new LBSolrClient.Req(new UpdateRequest().add("id", "1"), List.of(DEAD_HOST_1, DEAD_HOST_2)); + try (FailFirstEndpoint client = new FailFirstEndpoint(maybeSentException())) { + expectThrows(SolrServerException.class, () -> client.request(req)); + assertEquals(List.of(DEAD_HOST_1.getBaseUrl()), client.attempted); + } + } + + @Test + public void testQueryIsStillRetriedOnAnyIOException() throws Exception { + assertEquals( + List.of(DEAD_HOST_1.getBaseUrl(), DEAD_HOST_2.getBaseUrl()), + requestReturningAttemptedUrls(maybeSentException(), new QueryRequest())); + } +}
