SOLR-3284: let ConcurrentUpdateSolrClient report which docs failed to send

[serhiy-bzhezytskyy]

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

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

[dsmiley]

Very nice improvement!

[dsmiley]

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

[serhiy-bzhezytskyy]

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

[dsmiley]

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.

[serhiy-bzhezytskyy]

+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?

[serhiy-bzhezytskyy]

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]

sounds good; thanks

[serhiy-bzhezytskyy]

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.

[serhiy-bzhezytskyy]

Moving the tests into ConcurrentUpdateSolrClientTestBase (so they run for both clients) surfaced a real bug on the Jetty client: it coalesces several queued updates into a single <stream> request, so a failed batch was only reporting the first update's documents — the rest were lost silently. The JDK client sends one request per update, so it was already fine.

The fix: doSendUpdateStream now returns the response together with the updates it actually sent (SentStream), and the error handler is invoked for every affected update, not just the first one in the coalesced batch. On the Jetty side the OutStream — which already is the batch boundary — now owns the updates coalesced into it, rather than tracking them in a parallel list. A handler that throws on one update is logged and doesn't stop the others or the runner.

[dsmiley]

hmm; I'm concerned that this error tracking feature is going to hurt a benefit of CUSC -- memory efficiency (streaming a huge volume of updates).

What if we only track a designated ID field? The error handler could have a customizable overrideable method, defaulting to return the "id" field.

[serhiy-bzhezytskyy]

Switched it to report the failed document ids instead of the requests — onError(Throwable, List<String> ids, String collection) — so only ids are retained, not the documents, preserving the streaming memory efficiency.

The id comes from an overridable extractId(SolrInputDocument) defaulting to the id field, so a caller with a different uniqueKey can override it. When no handler is registered the documents are never read, so there's no overhead on the default path.

On the Jetty side the ids are collected as each doc is streamed, so a coalesced batch still reports every affected id on failure.