dsmiley commented on code in PR #4829:
URL: https://github.com/apache/solr/pull/4829#discussion_r3964665315
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java:
##########
@@ -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(
Review Comment:
might as well inline this now; it's a one-liner.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java:
##########
@@ -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
Review Comment:
No need to justify HttpSolrClient. It's natural we switch LBSolrClient to
that.
##########
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.
+ *
+ * <p>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) {
Review Comment:
IMO this is mostly pointless. The "was...Error" methods are coded very
simply and don't need a unit test.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -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
Review Comment:
this concerns me because there are plenty of non-update requests that are
not retry-able; most of the admin variety aren't. Any way,
https://issues.apache.org/jira/browse/SOLR-18341 hopes to tackle that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
