dsmiley commented on code in PR #2276: URL: https://github.com/apache/solr/pull/2276#discussion_r1498431204
########## solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java: ########## @@ -946,7 +939,6 @@ public void onFailure(Throwable throwable) { } }); } finally { - recoveryExec.shutdown(); Review Comment: By removing this, I see nothing here that will wait for this request to complete. But that was the semantics before, with the `future.get()` call. It kinda makes me wonder why we're even bothering with "async" request in the first place; couldn't we do a standard synchronous HTTP request? One little thing I did see was the "abort" call. But that could be done via using a FutureTask and calling `cancel(true)` on it. Basically, the field `prevSendPreRecoveryHttpUriRequest` because a FutureTask. Set it and call run() in `sendPrepRecoveryCmd`. I think it's sloppy/ugly to catch NPE in some places in this file, yuck; we could use a dummy initial FutureTask value. I think this might end up deferring any "executor" matters in this PR since we then wouldn't need an executor; right? It'd be a separate PR to improve Http2SolrClient executor initialization. ########## solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java: ########## @@ -911,18 +911,39 @@ private final void sendPrepRecoveryCmd(String leaderBaseUrl, String leaderCoreNa int readTimeout = conflictWaitMs + Integer.parseInt(System.getProperty("prepRecoveryReadTimeoutExtraWait", "8000")); - try (HttpSolrClient client = + var recoveryExec = + ExecutorUtil.newMDCAwareFixedThreadPool( + 1, new SolrNamedThreadFactory("sendPrepRecoveryCmd")); + try (Http2SolrClient client = recoverySolrClientBuilder( leaderBaseUrl, null) // leader core omitted since client only used for 'admin' request - .withSocketTimeout(readTimeout, TimeUnit.MILLISECONDS) + .withIdleTimeout(readTimeout, TimeUnit.MILLISECONDS) + .withExecutor(recoveryExec) .build()) { - HttpUriRequestResponse mrr = client.httpUriRequest(prepCmd); - prevSendPreRecoveryHttpUriRequest = mrr.httpUriRequest; - log.info("Sending prep recovery command to [{}]; [{}]", leaderBaseUrl, prepCmd); - - mrr.future.get(); + MDC.put("HttpSolrClient.url", baseUrl); Review Comment: Aaaah, you are imitating `org.apache.solr.client.solrj.impl.HttpSolrClient#httpUriRequest(org.apache.solr.client.solrj.SolrRequest<?>, org.apache.solr.client.solrj.ResponseParser)`. But there's no need because the Http2Solrclient natively supports an async method, which should be the place where any MDC/logging is specified. Note that below I question if we even need async logic in the first place! -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org