Skip to content

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

Open
serhiy-bzhezytskyy wants to merge 1 commit into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-7177-cusc-connection-error-url
Open

SOLR-7177: include the target URL when a ConcurrentUpdateSolrClient update fails to connect#4638
serhiy-bzhezytskyy wants to merge 1 commit into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-7177-cusc-connection-error-url

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor

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.

…pdate fails to connect

On a connection failure the runner passed the raw exception to the error handler
with no indication of which server it was talking to. Wrap a non-Solr exception
in a SolrServerException carrying the base URL, matching what the synchronous
clients already do. A RemoteSolrException already carries its URL, so it is
passed through unchanged.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant