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..39529c2dd99 --- /dev/null +++ b/changelog/unreleased/SOLR-18402-consolidate-retry-classification.yml @@ -0,0 +1,9 @@ +title: > + 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: + - name: Han Chan +links: + - name: SOLR-18402 + url: https://issues.apache.org/jira/browse/SOLR-18402 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..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 @@ -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; @@ -520,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; @@ -560,6 +565,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/impl/CloudSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java index 07c8af0c5cd..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 @@ -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,16 +203,6 @@ 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); - } - @Override public void close() { closed = true; @@ -722,7 +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 + || getHttpClient().wasRequestUnsent(exc); if (wasCommError || (exc instanceof RouteException @@ -754,7 +747,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 @@ -817,11 +811,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 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..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 @@ -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,26 @@ public Set getUrlParamNames() { return urlParamNames; } + /** + * 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); + } + + /** + * 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) + || 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..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,15 +17,10 @@ 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; 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; @@ -202,34 +197,8 @@ private void onFailedRequest( } listener.onFailure(e, false); } - } catch (SocketException e) { - if (!isNonRetryable || e instanceof ConnectException) { - 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 - && (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) || e instanceof RequestNotSentException) { + } 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 4a9d63cd375..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 @@ -20,10 +20,6 @@ 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 +39,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; @@ -210,7 +205,7 @@ public C getSolrClient() { public LBSolrClient build() { return new LBSolrClient(this) { @Override - protected SolrClient getClient(Endpoint endpoint) { + protected HttpSolrClient getClient(Endpoint endpoint) { return solrClient; } }; @@ -614,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( @@ -655,28 +641,8 @@ protected Exception doRequest( } throw e; } - } catch (SocketException e) { - if (!isNonRetryable || e instanceof ConnectException) { - 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 - && (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. + } catch (SolrServerException | IOException e) { + if (mayFailOver(baseUrl, e, isNonRetryable)) { ex = (!isZombie) ? makeServerAZombie(baseUrl, e) : e; } else { throw e; @@ -688,16 +654,25 @@ protected Exception doRequest( return ex; } - protected boolean isConnectException(Throwable t) { - if (t instanceof ConnectException || t instanceof HttpConnectTimeoutException) { + /** + * 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; } - // 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"); + Throwable rootCause = (e instanceof SolrServerException sse) ? sse.getRootCause() : e; + return !isNonRetryable + && (rootCause instanceof IOException || rootCause instanceof TimeoutException); } - 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/CloudSolrClientCacheTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java index f5b29e44ab2..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 @@ -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; @@ -121,7 +122,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 +139,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")); @@ -363,6 +451,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); @@ -431,7 +522,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..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 @@ -21,12 +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.util.IOUtils; import org.apache.solr.common.util.NamedList; import org.junit.Test; @@ -45,32 +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() { - @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); } } @@ -119,4 +125,55 @@ 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); + } + } + + /** + * 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}. + */ + @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..06479b174d7 --- /dev/null +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/SolrClientErrorClassificationTest.java @@ -0,0 +1,109 @@ +/* + * 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.SolrServerException; +import org.apache.solr.client.solrj.jetty.HttpJettySolrClient; +import org.eclipse.jetty.io.EofException; +import org.junit.Test; + +/** + * {@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. + */ +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))); + } + } + } +}