Copilot commented on code in PR #4714:
URL: https://github.com/apache/solr/pull/4714#discussion_r3730943072
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -206,9 +207,34 @@ public ClusterState getClusterState() {
return getClusterStateProvider().getClusterState();
}
+ /**
+ * Matches the message of the {@link IOException} the Jetty HTTP/2 client
produces when a stream
+ * or session fails without an HTTP response, e.g. {@code
cancel_stream_error/input_shutdown} when
+ * the server shuts down while a request is in flight. All HTTP/2 error code
names end in {@code
+ * _error} (see RFC 9113 section 7).
+ */
+ private static final Pattern HTTP2_STREAM_FAILURE_MESSAGE =
+ Pattern.compile("[a-z0-9_]+_error/.*");
+
/** Is this a communication error? We will retry if so. */
protected boolean wasCommError(Throwable t) {
- return t instanceof SocketException || t instanceof UnknownHostException;
+ return t instanceof SocketException
+ || t instanceof UnknownHostException
+ || wasHttp2StreamFailure(t);
+ }
+
+ /**
+ * HTTP/2 stream and session failures, such as the server closing the
connection while a request
+ * is in flight, surface from the Jetty client as a plain {@link
IOException} carrying only a
+ * message. No HTTP response was received, so treat them as communication
errors, just like a
+ * {@link SocketException}.
+ */
+ private static boolean wasHttp2StreamFailure(Throwable t) {
+ if (t == null || t.getClass() != IOException.class) {
+ return false;
+ }
Review Comment:
`t.getClass() != IOException.class` is overly strict and will miss
subclasses (e.g., if Jetty starts throwing a more specific `IOException`
subtype while keeping the same message format). Consider using `t instanceof
IOException` instead; the message-regex check is already specific enough to
avoid classifying unrelated IOExceptions.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -206,9 +207,34 @@ public ClusterState getClusterState() {
return getClusterStateProvider().getClusterState();
}
+ /**
+ * Matches the message of the {@link IOException} the Jetty HTTP/2 client
produces when a stream
+ * or session fails without an HTTP response, e.g. {@code
cancel_stream_error/input_shutdown} when
+ * the server shuts down while a request is in flight. All HTTP/2 error code
names end in {@code
+ * _error} (see RFC 9113 section 7).
+ */
+ private static final Pattern HTTP2_STREAM_FAILURE_MESSAGE =
+ Pattern.compile("[a-z0-9_]+_error/.*");
Review Comment:
The regex only matches lowercase error names. If the underlying client emits
uppercase/mixed-case tokens (common in protocol error naming), these failures
will not be classified as comm errors and won’t be retried. Consider broadening
the character class (e.g., include `A-Z`) or compiling with
`Pattern.CASE_INSENSITIVE`.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java:
##########
@@ -136,6 +136,50 @@ protected LBSolrClient createOrGetLbClient(HttpSolrClient
myClient) {
}
}
+ public void testHttp2StreamFailureIsRetriedAsCommError() throws Exception {
+ String collName = "gettingstarted";
+ Set<String> liveNodes = new HashSet<>(Set.of("192.168.1.108:8983_solr"));
+ AtomicReference<DocCollection> currentDoc = new
AtomicReference<>(loadCollection(collName, 1));
+ Map<String, ClusterState.CollectionRef> refs =
+ Map.of(
+ collName, new TestCollectionRef(currentDoc::get, new
AtomicInteger(), null, null, -1));
+ try (ClusterStateProvider provider = getStateProvider(liveNodes, refs);
+ RecordingCloudSolrClient client = new
RecordingCloudSolrClient(provider, 3)) {
+ // The Jetty HTTP/2 client surfaces a server shutting down mid-request
as a plain
+ // IOException with a "<h2 error code>/<reason>" message; it must be
retried
+ client.enqueue(
+ (req, cols) -> {
+ throw new SolrServerException(
+ "IOException occurred when talking to server",
+ new IOException("cancel_stream_error/input_shutdown"));
+ });
+ client.enqueue((req, cols) -> null);
+
+ NamedList<Object> resp = client.request(new
DummyUpdateRequest(collName), collName);
+ assertNotNull(resp);
Review Comment:
The test enqueues a successful response as `null` but then asserts the
response is non-null, which makes the test’s intent unclear and can become
brittle if `RecordingCloudSolrClient` behavior changes. Prefer enqueuing an
explicit non-null `NamedList` (e.g., an empty one) to make the success path
unambiguous.
--
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]
