serhiy-bzhezytskyy opened a new pull request, #4678: URL: https://github.com/apache/solr/pull/4678
Contributes to SOLR-9355. See [my comment there](https://issues.apache.org/jira/browse/SOLR-9355?focusedCommentId=18099778) for how this relates to the three questions in that issue's description. Split out of #4638 at markrmiller's suggestion — his note there was that the expectation of a `SolrServerException` is fragile, and that unrolling the exception chain to get the root cause is the better shape. ## The problem `SolrCmdDistributor.StdNode.checkRetry` asks whether a failure is retriable in two different ways, depending on which exception happens to be outermost: ```java if (err.e instanceof SolrServerException) { if (isRetriableException(((SolrServerException) err.e).getRootCause())) return true; // unwraps } else { if (isRetriableException(err.e)) return true; // top-level type only } ``` `isRetriableException` is a leaf test on `SocketException` / `SocketTimeoutException`. The async client reports a connection failure as a raw `ExecutionException` wrapping the socket cause, so it takes the second branch, the leaf test sees `ExecutionException`, and the update is not retried — the replica goes into recovery on a transient glitch. `ForwardNode.checkRetry` has the same asymmetry, narrowed to `ConnectException`. ## The change Both now scan the cause chain instead of checking one fixed position in it. **Why scan rather than call `getRootCause`** — the retriable frame is not always at either end. Jetty's `ClientConnector` wraps an underlying failure in a `SocketException` of its own, so going straight to the root cause walks past it; and the async client wraps from above, so the top-level type misses it too. **Why bounded** — a cause chain can be made cyclic, which `SolrException.getRootCause` carries a `// TODO: This doesn't handle cause loops` about. Real chains are a few frames deep; the cap only guards the pathological case. **What does not change** - the retriable set: still `SocketException`/`SocketTimeoutException` for `StdNode`, `ConnectException` for `ForwardNode` - the retry bound: still `Req.shouldRetry`, requiring `retries < maxRetries` and excluding delete-by-query ## Tests `CheckRetryUnrollTest`, 10 tests, written before the fix. **Two fail on unmodified `main`** — the two shapes where a retriable cause is wrapped in something that is not a `SolrServerException` — and pass after. The rest are guards rather than new assertions: the two shapes that already worked, the `MAX_CAUSE_DEPTH` bound against a cyclic chain, the `retry == false` gate, and three negative controls confirming nothing new became retriable. One of them records a limit rather than hiding it: `testClosedChannelExceptionIsStillNotRetriableEitherWay`. `ClosedChannelException` is what the JDK transport reports as the root cause of a dropped update connection, and it is retriable nowhere in Solr today. That is a separate question — see the flag on SOLR-9355 — and this PR deliberately does not widen anything. ## Coverage gap worth naming `ForwardNode.hasConnectExceptionInChain` has no unit test here. `ForwardNode` needs a live `ZkStateReader` to construct, so rather than mock it I noted the limitation in the test class javadoc. It is exercised by `SolrCmdDistributorTest` (which passes, including `testForwardNodeWontRetrySocketError`), but that is inherited coverage, not coverage added for the new branch. I can add a heavier test if you'd rather have it explicit. ## Checks `CheckRetryUnrollTest` + `SolrCmdDistributorTest` green locally; `spotlessJavaCheck`, `ecjLintMain` and `logchangeLint` clean. Changelog entry generated with `gradlew writeChangelog`. Note `./gradlew check -x test` fails on this branch with ~102 RAT "Unknown license" warnings — including `AGENTS.md` itself and gradle build caches. I verified the identical 102 on a pristine worktree at `origin/main`, so it is pre-existing and unrelated to 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]
