dsmiley commented on code in PR #2276: URL: https://github.com/apache/solr/pull/2276#discussion_r1506994591
########## solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java: ########## @@ -633,9 +631,10 @@ public final void doSyncOrReplicateRecovery(SolrCore core) throws Exception { .getSlice(cloudDesc.getShardId()); try { - prevSendPreRecoveryHttpUriRequest.cancel(); + prevSendPreRecoveryHttpUriRequest.cancel(true); } catch (NullPointerException e) { // okay + log.info("Failed to abort the Prep Recovery command as it has not been sent yet."); Review Comment: No need to log; this is very normal and non-interesting. IMO even better to either ensure this is non-null (use an initial dummy value), or to check null before even calling cancel. It's kinda sloppy to catch NPE. Okay if you prefer to defer this! ########## solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java: ########## @@ -175,24 +174,21 @@ public final void setRecoveringAfterStartup(boolean recoveringAfterStartup) { this.recoveringAfterStartup = recoveringAfterStartup; } - /** Builds a new HttpSolrClient for use in recovery. Caller must close */ - private HttpSolrClient.Builder recoverySolrClientBuilder(String baseUrl, String leaderCoreName) { + private Http2SolrClient.Builder recoverySolrClientBuilder(String baseUrl, String leaderCoreName) { Review Comment: I observe that you changed UpdateShardHandler to have a SolrClient instead of an HttpClient for the recovery ops. Nice; note that this is a higher level abstraction. A ramification of this is that RecoveryStrategy shouldn't need to create brand new SolrClient instances because now we have one already pre-made! Thus don't need to close them either. I assume they all need the same configuration? Also, I looked at the comment below RE SOLR-13605 and it's obsolete -- SOLR-13605 is closed yet this comment pre-dated it. ########## solr/CHANGES.txt: ########## @@ -111,6 +111,8 @@ Improvements * SOLR-16138: Throw a exception when issuing a streaming expression and all cores are down instead of returning 0 documents. (Antoine Bursaux via Eric Pugh) +* SOLR-16505: Switch UpdateShardHandler.getRecoveryOnlyHttpClient to Jetty HTTP2 (Sanjay Dutt, David Smiley) Review Comment: IMO this better fits in "Other Changes" as it's more of a refactoring than any kind of improvement that would be noticed. I suggest using words a user might slightly more likely understand like "Switch replica recovery PREPRECOVERY command to Jetty HTTP2". ########## solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java: ########## @@ -915,18 +911,15 @@ private final void sendPrepRecoveryCmd(String leaderBaseUrl, String leaderCoreNa int readTimeout = conflictWaitMs + Integer.parseInt(System.getProperty("prepRecoveryReadTimeoutExtraWait", "8000")); - try (HttpSolrClient client = + try (Http2SolrClient client = Review Comment: minor: please broaden the type to just SolrClient so we aren't overly specific (e.g. List vs ArrayList) -- 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