dsmiley commented on code in PR #4632: URL: https://github.com/apache/solr/pull/4632#discussion_r3564559919
########## changelog/unreleased/solr-3284-cusc-failed-docs.yml: ########## @@ -0,0 +1,12 @@ +# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc + +title: > + 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 Review Comment: 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 ########## solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateBaseSolrClient.java: ########## @@ -71,6 +71,24 @@ public abstract class ConcurrentUpdateBaseSolrClient extends SolrClient { protected StallDetection stallDetection; + private final UpdateErrorHandler errorHandler; + + /** + * 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. Review Comment: I don't recommend a JIRA issue reference unless there's a complex issue that's worth remembering going forward. Neither apply. ########## solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateBaseSolrClient.java: ########## @@ -158,6 +176,7 @@ protected ConcurrentUpdateBaseSolrClient(Builder builder) { this.basePath = builder.baseSolrUrl; this.defaultCollection = builder.defaultCollection; this.pollQueueTimeMillis = builder.pollQueueTimeMillis; + this.errorHandler = builder.errorHandler; Review Comment: 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. ########## solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateJdkSolrClientTest.java: ########## @@ -71,6 +75,54 @@ public ConcurrentUpdateBaseSolrClient outcomeCountingConcurrentClient( .build(); } + /** + * SOLR-3284: when a batch fails to reach the server, the caller should be able to learn which + * documents did not make it. A handler registered via {@link + * ConcurrentUpdateBaseSolrClient.Builder#withErrorHandler} receives the failed UpdateRequest (and + * thus its documents), which the old handleError(Throwable) threw away. + */ + @Test + public void testFailedDocsAreRecoverableViaErrorHandler() throws Exception { + TestServlet.clear(); + TestServlet.setErrorCode(500); // every request fails server-side + + String serverUrl = solrTestRule.getBaseUrl() + "/cuss/foo"; + // "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); Review Comment: nit: I'd prefer we no longer speak of "http2client" as it's become needless in in 2026 in Solr. Just remove the 2. ########## solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateBaseSolrClient.java: ########## @@ -544,6 +573,24 @@ public void handleError(Throwable ex) { log.error("error", ex); } + /** + * Called when an error occurs sending a batch, with the {@link Update} that failed. The default + * implementation invokes the {@link UpdateErrorHandler} registered via {@link + * Builder#withErrorHandler} if one was provided (passing the failed request and its documents), + * and otherwise falls back to {@link #handleError(Throwable)}. Subclasses may still override this + * to recover the documents that did not reach the server. See SOLR-3284. + * + * @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) { Review Comment: is there value in this being public or protected, presumably for a subclasser? -- 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]
