SOLR-18402: Consolidate wasRequestUnsent / wasCommError (retry) logic

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

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).

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<Object> 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> 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();

solr/solrj/src/java/org/apache/solr/client/solrj/SolrClient.java

@@ -1194,6 +1194,23 @@ public final NamedList<Object> 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

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<Object> 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<Object> 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<Object> 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;
+            }
           }
         }
       }

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<Object> 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 {

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<String> 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
    */

solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBAsyncSolrClient.java

@@ -17,15 +17,13 @@
 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;
@@ -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,17 +217,15 @@ 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);
       } else {
         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);

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,31 +669,27 @@ 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);
     }
 
     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() {