serhiy-bzhezytskyy opened a new pull request, #4632: URL: https://github.com/apache/solr/pull/4632
https://issues.apache.org/jira/browse/SOLR-3284 # Description `ConcurrentUpdateSolrClient` sends updates asynchronously on background threads. When a batch fails, the only signal a caller gets is `handleError(Throwable)` — the exception, but not *which* documents didn't make it to the server. So a caller can't route the failed documents anywhere (retry queue, dead-letter topic); it only knows that *something* in the batch failed. This is the pain SOLR-3284 has tracked since 2012, and it's still present on `main` (`ConcurrentUpdateBaseSolrClient`). We hit it in production indexing into Solr via the Builder, which is what prompted this. The 2016 discussion on the JIRA (David Smiley, Mark Miller) converged on a Builder-configurable error handler, ideally a lambda. This implements that. # Changes - **New `UpdateErrorHandler` functional interface**: `onError(Throwable ex, UpdateRequest request, String collection)`. It exposes the public `UpdateRequest` (and thus its documents), not the `protected Update` record, so it's usable by external callers. The caller reads whatever field is their `uniqueKey` — the client doesn't assume one (no hardcoded `id`). - **New `Builder.withErrorHandler(...)`.** The default `handleError(Throwable, Update)` invokes the handler if one is registered, otherwise falls back to `handleError(Throwable)`. With no handler the behavior is unchanged (logs), so this is backward compatible; existing `handleError(Throwable)` overrides keep working. - **Restructured the runner loop** so the failing `Update` is in scope and every failure path — HTTP error status *and* a thrown exception — reports it to the handler. Usage: ```java new ConcurrentUpdateJdkSolrClient.Builder(url, httpClient) .withErrorHandler((ex, request, collection) -> { for (SolrInputDocument doc : request.getDocuments()) { // route the doc that didn't reach the server to retry / DLQ } }) .build(); ``` # Two decisions I'd flag for review 1. **Default behavior.** I kept it **optional + log-by-default** for backward compatibility. The 2016 discussion leaned toward surfacing errors by default (throw when no handler is set, like `SafeConcurrentUpdateSolrClient`). I went with compatibility, but I'm happy to switch to throw-by-default if that's preferred — it's a small change. 2. **Runner-loop behavior.** To pass the failing request to the handler I catch failures per-update *inside* the runner loop. A side effect is that the runner now continues to the next batch instead of exiting the loop on error. This seems better (one bad batch no longer stalls the runner), but it is a behavior change and I want it visible rather than buried. # Side finding (not fixed here) While testing I hit a pre-existing NPE: `RemoteSolrException(host, code, remoteError)` throws when `remoteError` is `null`, because the constructor does `Map.of("remoteError", remoteError)` and `Map.of` rejects null values. It's triggered when the server returns an error body that doesn't parse. Unrelated to this change and probably deserves its own issue — flagging it rather than expanding scope here. # Testing - New `ConcurrentUpdateJdkSolrClientTest#testFailedDocsAreRecoverableViaErrorHandler`: registers a handler via the Builder, sends 5 docs to a server returning 500, asserts all 5 doc ids are recovered. - `./gradlew :solr:solrj:check :solr:solrj-jetty:check` pass (solrj-jetty shares the base class). -- 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]
