dsmiley commented on code in PR #2276:
URL: https://github.com/apache/solr/pull/2276#discussion_r1508136097


##########
solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java:
##########
@@ -630,11 +628,8 @@ public final void doSyncOrReplicateRecovery(SolrCore core) 
throws Exception {
                 .getCollection(cloudDesc.getCollectionName())
                 .getSlice(cloudDesc.getShardId());
 
-        try {
+        if (prevSendPreRecoveryHttpUriRequest != null) {

Review Comment:
   I see this is correct because prevSendPreRecoveryHttpUriRequest never 
becomes null once it is non-null.  Otherwise a concurrent set to null would 
cause an NPE here if timed just right.  But this is super subtle!  It'd be 
clearer, even if slightly more code, to save the field access to a local 
variable where you check it then -- a typical pattern when a field can be 
accessed concurrently.  Perhaps there's a one-liner approach using 
`Optional.ofNullable(field).ifPresent(f -> f.cancel(true))`



-- 
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

Reply via email to