SOLR-3284: let ConcurrentUpdateSolrClient report which docs failed to send by serhiy-bzhezytskyy · Pull Request #4632 · apache/solr

serhiy-bzhezytskyy · 2026-07-10T14:55:46Z

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:

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

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.
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).

… send Add a Builder-configurable error handler so callers can recover the documents of a failed update batch. handleError(Throwable) alone only gave the exception, not which documents never reached the server, so callers could not route the failed docs to a retry queue or DLQ. - New UpdateErrorHandler functional interface: onError(Throwable, UpdateRequest, String collection). Exposes the public UpdateRequest (and thus its documents), not the protected Update record, so it is usable by external callers. The caller reads whatever field is their uniqueKey; the client does not assume one. - New Builder.withErrorHandler(...). Default handleError(Throwable, Update) invokes the handler if registered, else falls back to handleError(Throwable). No handler = existing log behavior, so this is backward compatible. - Restructure the runner loop so the failing Update is in scope and every failure path (HTTP status and thrown exception) reports it. Implements the Builder-configurable handler discussed on SOLR-3284.

dsmiley

Very nice improvement!

dsmiley · 2026-07-11T16:11:36Z

+  ConcurrentUpdateSolrClient can now report which documents failed to reach the server.
+  Register a handler via the Builder's withErrorHandler(...) to recover the documents of a
+  failed batch, for example to route them to a retry queue or dead-letter topic.
+type: added


That's the best changelog entry I've seen in a long time; thank you!

I think our project's embrace of the 3rd party "changelog" with it's choice of wording of "title" has led to a deterioration of changelog informative quality compared to before. You are bucking that trend. Thanks again. Our project seriously needs to find a way to use a different label here like "summary". CC @janhoy

Thanks — that means a lot. Agree the "title" label nudges people toward terse one-liners; "summary" would invite more.

dsmiley · 2026-07-11T16:12:50Z

+   * Callback invoked when an update batch fails to reach the server, receiving the request (and its
+   * documents) that did not make it. Register one via {@link Builder#withErrorHandler}. This is the
+   * hook that lets a caller recover the documents of a failed batch (e.g. route them to a retry
+   * queue). See SOLR-3284.


I don't recommend a JIRA issue reference unless there's a complex issue that's worth remembering going forward. Neither apply.

Done, removed the references.

dsmiley · 2026-07-11T16:15:21Z

+   * @param ex the error that occurred
+   * @param update the update (request + collection) that failed to send; may be null
+   */
+  public void handleError(Throwable ex, Update update) {


is there value in this being public or protected, presumably for a subclasser?

Made it protected. It takes the protected Update record so it was never callable externally anyway; protected is the honest visibility for a subclasser.

Follow-up — correcting myself, made it private. The 2-arg handleError(Throwable, Update) is really just internal dispatch: if an errorHandler is registered call it, else fall back to handleError(Throwable). There's no behavior in it worth overriding — subclassers already have handleError(Throwable), and callers now have the lambda. Keeping the bridge private means one subclass hook and one injection point, rather than a second overridable error method with murky precedence between them. And if the handler is heading toward being the preferred mechanism, I'd rather not open a new subclass surface on the way there.

dsmiley · 2026-07-11T16:22:38Z

    this.basePath = builder.baseSolrUrl;
    this.defaultCollection = builder.defaultCollection;
    this.pollQueueTimeMillis = builder.pollQueueTimeMillis;
+    this.errorHandler = builder.errorHandler;


IMO we probably want to make this new errorHandler the preferred mechanism... maybe even eventually mandatory in 11.

If we agree on this direction for 11 (I'm +1), then here we can use DeprecationLog to warn that a user forgot to pass an erroHandler, that it'll eventually be mandatory.

+1 on making the handler the preferred mechanism, and mandatory in 11. One wrinkle: DeprecationLog lives in solr-core (org.apache.solr.logging), so solrj can't reach it — wrong direction in the module graph. I can do the equivalent with a log.warn here when a client is built without a handler (and hasn't overridden handleError), worded as "will be required in a future release". Want me to go that way, or is there a solrj-side deprecation helper you'd prefer?

Confirmed there's no solrj-side deprecation helper — solrj only has @deprecated + ad-hoc log.warn, and DeprecationLog is core-only as noted. The one thing DeprecationLog adds over a bare log.warn is log-once dedup + the org.apache.solr.DEPRECATED.* logger prefix. To avoid warning on every client construction I'd add a small log-once guard here in the client. If the project later wants solrj to share core's deprecation convention, a tiny solrj-side helper would be a clean follow-up — but I'd keep this PR scoped to the guard. Sound good?

dsmiley · 2026-07-11T16:26:30Z

+    // "id" here is this test's own uniqueKey; a real caller uses whatever their schema defines.
+    List<String> failedIds = new CopyOnWriteArrayList<>();
+
+    try (var http2Client = solrClient(null);


nit: I'd prefer we no longer speak of "http2client" as it's become needless in in 2026 in Solr. Just remove the 2.

Done — renamed to httpClient.

…y, rename - Remove the "See SOLR-3284" references from javadoc/comments (per review: a JIRA ref isn't warranted here). - handleError(Throwable, Update) is now protected instead of public; it takes the protected Update record, so it was never callable externally anyway — protected is the accurate visibility for subclassers. - Rename the test's http2Client local to httpClient.

…atch)

serhiy-bzhezytskyy · 2026-07-12T05:26:42Z

Documented the handler's threading contract, with a test pinning it: the handler is invoked on runner threads, possibly concurrently — implementations must be thread-safe and should return quickly (a slow handler slows update throughput). An exception thrown by the handler is logged and does not stop the client — the runner's existing safety net already contains it; the test locks that promise in. No ordering guarantees across failures. No behavior change in this commit.

dsmiley · 2026-07-12T18:32:37Z

+   * Internal dispatch for a failed batch: if an {@link UpdateErrorHandler} was registered via
+   * {@link Builder#withErrorHandler} it is invoked with the failed request and its documents,
+   * otherwise we fall back to {@link #handleError(Throwable)}.
   *
   * @param ex the error that occurred
   * @param update the update (request + collection) that failed to send; may be null
   */


so much words for a private method that reads plainly in fewer. Please remove.

dsmiley · 2026-07-12T18:44:15Z

as the changes in this PR are not strictly related to JDK HttpClient, couldn't you put this test in ConcurrentUpdateSolrClientTestBase instead?

github-actions Bot added client:solrj tests labels Jul 10, 2026

dsmiley reviewed Jul 11, 2026

View reviewed changes

dsmiley requested a review from markrmiller July 11, 2026 16:27

serhiy-bzhezytskyy added 3 commits July 11, 2026 19:36

SOLR-3284: make handleError(Throwable, Update) private (internal disp…

3507713

…atch)

SOLR-3284: document errorHandler threading contract; add test pinning it

5d9bf45

dsmiley reviewed Jul 12, 2026

View reviewed changes

Uh oh!

Conversation

serhiy-bzhezytskyy commented Jul 10, 2026

Description

Changes

Two decisions I'd flag for review

Side finding (not fixed here)

Testing

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-bzhezytskyy commented Jul 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants