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