SOLR-7177: include the target URL when a ConcurrentUpdateSolrClient update fails to connect

[serhiy-bzhezytskyy]

On a connection failure, ConcurrentUpdateBaseSolrClient's runner passed the raw exception to the error handler with no indication of which server it was talking to — the original SOLR-7177 complaint. The synchronous clients already wrap connection failures with the URL (HttpJdkSolrClient, HttpSolrClient); this brings the async path in line.

The runner now wraps a non-Solr exception in a SolrServerException carrying the base URL. A RemoteSolrException already carries its URL, so it passes through unchanged.

While verifying this I traced the SolrCloud distribution path (SolrCmdDistributor.checkRetry via ErrorReportingConcurrentUpdateSolrClient) and it turned out to matter for retries, not just logging. checkRetry inspects the exception like this:

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
}

On the Jetty path a connection failure arrives as a raw ExecutionException, which hits the else branch and isn't recognized as retriable — so a transient failure to a replica isn't retried. Wrapping it as a SolrServerException puts it in the shape checkRetry unwraps via getRootCause(), so a socket-rooted cause (e.g. ConnectException) is now correctly seen as retriable. This never stops a previously-retried case from retrying; it starts retrying the socket-rooted ones the async path was dropping. That overlaps with SOLR-9355.

Tests added in ConcurrentUpdateSolrClientTestBase (both transports): the reported error identifies the target server, and it's a SolrServerException preserving the original cause so checkRetry can unwrap it.

@markrmiller — you had reservations on this ticket back in 2015 (the client knows its own URL). The retry-path finding above is the part I'd flag: the async CUSC path wasn't producing the SolrServerException shape checkRetry was built to unwrap. Happy to adjust the approach.

[serhiy-bzhezytskyy]

the CI red here is two tests that this PR doesn't touch (it only adds the target URL to a CUSC connection-error log). checked both on clean main with the same CI seed:

• `TestGracefulJettyShutdown.testSingleShardInFlightRequestsDuringShutDown` — fails 3 of 5 runs on clean main (seed 284231B7A8B0E347), with ClosedChannelException on an in-flight request during shutdown. so it looks like a pre-existing failure in that test (added under SOLR-18285), not related to this change.
• `DistributedFacetPivotSmallTest` — passed 5 of 5 on clean main with the same seed; its CI failure was an ObjectReleaseTracker leak (HttpJettySolrClient / SolrMetricsContext) i couldn't reproduce locally.

a re-run should get this green.

[serhiy-bzhezytskyy]

Are there any objections to its merger? I am ready to resolve them, just let me know. Thanks

[markrmiller]

Seems fine to me.

Not fully related to this issue, but on a custom branch many years ago, I dealt with that fragile expectation of a SolrServerException by just unrolling the exception chain to get the root cause rather than hoping the exception is a SolrServerException.

[serhiy-bzhezytskyy]

Thanks Mark. On the exception chain — you're right that the assumption is fragile, and this PR arguably adds to it: the new branch wraps a Throwable in a SolrServerException so the message can carry basePath, which means callers still have to know to look for that type.

Unrolling to the root cause is the better shape. I'd rather keep it out of this PR, since the change here is meant to be the narrow "which node was it?" fix, and unrolling changes what every handleError caller observes — the three call sites are all in ConcurrentUpdateBaseSolrClient, but the behaviour is public through the overridable handleError(Throwable), so it's a small diff with a visible contract change.

Let me open a separate issue for it and reference your note. It would also help UpdateErrorHandler (SOLR-3284, and SOLR-18308 which proposes making it the preferred mechanism) report the real cause instead of the wrapper.

Is that the split you'd expect, or would you rather see them together?

[markrmiller]

Yeah, I'd split it out.

[serhiy-bzhezytskyy]

Created as SOLR-9355 rather than a new issue — Erick Erickson opened that one in 2016 asking for exactly this retry, and it turned out your note explains why it never fired. PR is #4678.

What it came to: checkRetry asked the retriability question two different ways depending on which exception was outermost, unwrapping only when it was a SolrServerException. Since the async client reports a connection failure as a raw ExecutionException, that branch leaf-tested the wrapper and the update wasn't retried. Both node types now scan the cause chain instead; the retriable set and the retry bound are unchanged.

One thing I got wrong in my earlier reply here: I said unrolling to the root cause was the better shape. It isn't quite — Jetty's ClientConnector wraps an underlying failure in a SocketException of its own, so getRootCause can walk straight past the frame that matters. Scanning the chain covers both that and the wrapped-from-above case.

Your observation also generalised further than the split. Five places in Solr decide retriability with five different sets, and ConnectException is the only type all of them agree on — that's on SOLR-9355 as a separate design flag, not something I'm widening anywhere.

Nothing needed from you here; #4638 is unchanged.