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: [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]
