You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "serhiy-bzhezytskyy (via GitHub)" <gi...@apache.org> on 2026/07/16 08:20:25 UTC
[PR] SOLR-17764: retry CloudSolrClient requests failing with ClosedChannelException [solr]
serhiy-bzhezytskyy opened a new pull request, #4643:
URL: https://github.com/apache/solr/pull/4643
While looking into the graceful-shutdown test failures (SOLR-17764), I noticed `CloudSolrClient.wasCommError()` only classifies `SocketException` and `UnknownHostException` as communication errors worth retrying.
A connection dropped mid-request surfaces as a `java.nio.channels.ClosedChannelException` (for example an HTTP/2 GOAWAY when a node shuts down). That was not treated as a comm error, so the request was not retried even though `maxRetries` allowed it — the logs show `commError=false` and no retry. This is the kind of "connection error should be retried" case a couple of committers pointed at on SOLR-17764.
Fix: treat `ClosedChannelException` as a communication error, alongside the existing two, so the request is retried on another node when one is available. Added a unit test for the classification.
Scope / honesty: this does **not** by itself fix the graceful-shutdown failures in SOLR-17764.
- The underlying failure is a Jetty HTTP/2 client bug (jetty#15368) that discards an already-sent response on connection close. I confirmed it still reproduces on Jetty 12.1.11 (bumped + relocked + ran the failing seed 10x), so a version bump does not help.
- And for the single-node case in `TestGracefulJettyShutdown`, retry cannot help because there is no other node to route to.
So this is a standalone retry-robustness improvement that is correct on its own merits, not a fix for the graceful-shutdown issue. Details and experiments posted on SOLR-17764.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17764: retry CloudSolrClient requests failing with ClosedChannelException [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley closed pull request #4643: SOLR-17764: retry CloudSolrClient requests failing with ClosedChannelException
URL: https://github.com/apache/solr/pull/4643
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17764: retry CloudSolrClient requests failing with ClosedChannelException [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #4643:
URL: https://github.com/apache/solr/pull/4643#issuecomment-5308328263
> One caveat worth stating against this PR, not for it. ClosedChannelException is not a reliable signal of a transient peer failure.
Right. I'm -1 on checking ClosedChannelException on wasCommError (was this PR is doing) because I believe the intent of that method is to ascertain if the request was submitted or not. That exception isn't a reliable indicator.
Nonetheless I think we can improve retry-ability separately from exception inspection. I filed https://issues.apache.org/jira/browse/SOLR-18341 and https://issues.apache.org/jira/browse/SOLR-18343 You are welcome to take them on if you like. There may be other ways ... like by checking if the payload has been transmitted yet.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17764: retry CloudSolrClient requests failing with ClosedChannelException [solr]
Posted by "serhiy-bzhezytskyy (via GitHub)" <gi...@apache.org>.
serhiy-bzhezytskyy commented on PR #4643:
URL: https://github.com/apache/solr/pull/4643#issuecomment-5088819339
Are there any objections to its merger? I am ready to resolve them, just let me know. Thanks
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17764: retry CloudSolrClient requests failing with ClosedChannelException [solr]
Posted by "serhiy-bzhezytskyy (via GitHub)" <gi...@apache.org>.
serhiy-bzhezytskyy commented on PR #4643:
URL: https://github.com/apache/solr/pull/4643#issuecomment-5106454991
Some context I turned up while looking at a neighbouring retry problem, in case it helps this move — or argues against it.
**The gap this PR closes has an unanswered question behind it.** On SOLR-11881 ("Retry update requests from leaders to replicas"), Varun Thacker asked outright while adding `SocketTimeoutException` to `isRetriableException`:
> Q: What all exceptions should we retry on? Currently in the patch we have SocketException / NoHttpResponseException
That question was never answered in the thread, and the sets we have today look like its residue. Right now:
| | retriable set |
|---|---|
| `CloudSolrClient.wasCommError` (:211) | `SocketException`, `UnknownHostException` |
| `SolrCmdDistributor.isRetriableException` (:591) | `SocketException`, `SocketTimeoutException` |
So `SocketTimeoutException` is retriable on the distributor path but not the cloud-client path, and `UnknownHostException` the reverse. Neither asymmetry looks deliberate, and `ClosedChannelException` — the root cause the JDK transport reports for a dropped update connection — is in neither.
**The safety argument for widening is already on the record.** Tomás asked why a test asserted `SocketException` was deliberately not retried, and Mark Miller answered:
> Yes it was, because it can happen mid request and we don't know if the request failed or succeeded. Given we are counting on versions for retry though, this actually shouldnt matter, so that should be fine.
That reasoning applies unchanged here: a dropped connection is ambiguous in exactly the same way, and versioning is what makes the replay harmless.
**One caveat worth stating against this PR, not for it.** `ClosedChannelException` is not a reliable signal of a transient peer failure. Jetty *manufactures* one as the payload of a retry it already knows is safe — `HttpConnectionOverHTTP2.send()` returns `new SendFailure(new ClosedChannelException(), true)` when the connection is already closed, before any frame is written — and also surfaces the same type on terminal failures. Notably Jetty's own client contains no `instanceof ClosedChannelException` anywhere; retriability there is a boolean set at the throw site by code that knows nothing was transmitted, not a type test at the catch site.
So the type tells us "a channel closed", not "the update didn't land". On this path that is fine for the reason Mark gives above, but it's worth knowing the type is doing less work than it appears to.
Two smaller things I noticed nearby, both pre-existing and not for this PR:
- `SolrException.getRootCause` carries `// TODO: This doesn't handle cause loops` (:152). `PeerSync.connectTimeoutExceptionInChain` (:443) hand-rolls the same unbounded walk. A cause chain can be made cyclic, so both can spin.
- `LBSolrClient.isConnectException` (:692) falls back to `t.getClass().getName().endsWith("ConnectTimeoutException")` to avoid a hard dependency on a specific HTTP client. Honest about its reason, but it suggests the missing piece is a shared way to ask this question rather than five local answers.
I can write either of those up separately if useful; neither needs to block this change.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org