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: [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]
