dsmiley commented on code in PR #4829:
URL: https://github.com/apache/solr/pull/4829#discussion_r3916515870
##########
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.
Review Comment:
I don't think this is worth putting in the ref guide. It's a detail and
doesn't change how people use SolrJ.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/SolrClient.java:
##########
Review Comment:
These seem HttpSolrClient worthy and not generalized to any SolrClient (e.g.
not EmbeddedSolrServer). Even not worthy of CloudSolrClient since it's really
the backing HttpSolrClient, which CSC exposes.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java:
##########
@@ -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) {
Review Comment:
An implicit outcome of SOLR-18402, I think, is to massively simplify catch
blocks that currently are overly complex. Adding an IOException here and not
simplifying or generalizing the previous ones is counter to this direction.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/SolrClient.java:
##########
@@ -1194,12 +1194,27 @@ 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;
+ }
+
Review Comment:
I generated this JIRA description with AI, and I did read it. But I confess
now (and I recall then as well), I'm confused on the distinction between these
2 methods. It's not clear to me why we need a distinction between these two.
Feel free to help me figure this out ;-)
--
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]
