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

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<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;
@@ -560,6 +565,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/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<Object> 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<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
@@ -817,11 +811,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;
@@ -227,6 +228,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;
@@ -358,6 +362,26 @@ public Set<String> 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
    */

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

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