serhiy-bzhezytskyy commented on code in PR #4632: URL: https://github.com/apache/solr/pull/4632#discussion_r3564610562
########## 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: Thanks — that means a lot. Agree the "title" label nudges people toward terse one-liners; "summary" would invite more. ########## 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: Done, removed the references. ########## 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: Made it protected. It takes the protected Update record so it was never callable externally anyway; protected is the honest visibility for a subclasser. ########## 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: Done — renamed to httpClient. ########## 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: +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? -- 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]
