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..b095a396737 --- /dev/null +++ b/changelog/unreleased/SOLR-18402-consolidate-retry-classification.yml @@ -0,0 +1,9 @@ +title: > + 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 +links: + - name: SOLR-18402 + url: https://issues.apache.org/jira/browse/SOLR-18402 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). 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 77e1f7e3f1a..af8b087d239 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.Collection; import java.util.Iterator; @@ -50,6 +51,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.ContentStream; @@ -85,6 +87,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; @@ -528,9 +531,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; @@ -568,6 +573,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/src/java/org/apache/solr/client/solrj/SolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/SolrClient.java index e83ce93f7a5..9e3b9d2a1aa 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 f5a250fb2e8..cacea7286ce 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,14 +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. - */ - protected boolean wasCommError(Throwable t) { - return SolrException.hasCause(t, SocketException.class) - || SolrException.hasCause(t, UnknownHostException.class) - || SolrException.hasCause(t, RequestNotSentException.class); + /** 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 @@ -719,6 +717,11 @@ protected NamedList requestWithRetryOnStaleState( : SolrException.ErrorCode.UNKNOWN.code; final boolean wasCommError = 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); if (wasCommError || (exc instanceof RouteException @@ -750,7 +753,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 @@ -813,11 +817,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 (mayReplay) { + // looks like we couldn't reach the server because the state was stale == retry + stateWasStale = true; + } } } } 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 722b241cef6..c539d86bc37 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; @@ -229,6 +230,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 fccc4357fcd..8f1e8766c2b 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; @@ -367,6 +371,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/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..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 @@ -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,15 +669,20 @@ 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; } 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); } @@ -688,15 +690,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/CloudSolrClientCacheTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java index 0b671fb81d4..f713b86b9cb 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 @@ -45,6 +45,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; @@ -123,7 +124,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"); @@ -140,6 +141,93 @@ 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> 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; + } + } + // 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))); + + // 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"); + + expectThrows(SocketException.class, () -> cloudClient.request(update, collName)); + assertEquals( + "an update that may have been applied must not be replayed", 1, lbhttpRequestCount.get()); + } + } + + /** + * {@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 testUpdateIsNotRetriedOnRouteExceptionWith503() throws Exception { + String collName = "gettingstarted"; + Set livenodes = new HashSet<>(); + Map refs = 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; + } + }.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 -> { + lbhttpRequestCount.incrementAndGet(); + return new CloudSolrClient.RouteException( + SolrException.ErrorCode.SERVICE_UNAVAILABLE, shardFailures, Map.of()); + }); + + UpdateRequest update = new UpdateRequest().add("id", "123", "desc", "Something 0"); + expectThrows( + CloudSolrClient.RouteException.class, () -> cloudClient.request(update, collName)); + assertEquals( + "a 503 may follow partial success, so it 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")); @@ -365,6 +453,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); @@ -433,7 +524,7 @@ public ClusterStateProvider getClusterStateProvider() { @Override public HttpSolrClient getHttpClient() { - throw new UnsupportedOperationException(); + return httpClient; } @FunctionalInterface 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..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 @@ -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 { @@ -119,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"))); + } } 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..7bc5ef83317 --- /dev/null +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/SolrClientErrorClassificationTest.java @@ -0,0 +1,128 @@ +/* + * 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(), + // 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))); + } + } + } + + /** 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"))); + } +}