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