bruno-roustant commented on code in PR #2038:
URL: https://github.com/apache/solr/pull/2038#discussion_r1371992238


##########
solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java:
##########
@@ -883,8 +836,7 @@ private void checkReplicationTracker(UpdateCommand cmd) {
     }
   }
 
-  private List<SolrCmdDistributor.Node> getCollectionUrls(
-      String collection, EnumSet<Replica.Type> types, boolean onlyLeaders) {

Review Comment:
   I looked at the git history.
   This code comes from a refactor in 2019 of DistributedUpdateProcessor to 
extract the code specific to cloud. At that time this method signature already 
contained the boolean.
   Looking earlier, I saw that the boolean was added in 2018 in a commit “Make 
massive improvements to the tests”. It added the boolean and modified the 
single call to pass true. Well, I believe it was a perf optimization to always 
limit to leaders. At that time the intent was clearly to ignore the dead code, 
and I think the dead code should have been removed from the method instead of 
adding this boolean.
   So after looking at the context, I’m +1 to remove this dead code.



##########
solr/core/src/java/org/apache/solr/update/processor/DistributedZkUpdateProcessor.java:
##########
@@ -883,8 +836,7 @@ private void checkReplicationTracker(UpdateCommand cmd) {
     }
   }
 
-  private List<SolrCmdDistributor.Node> getCollectionUrls(
-      String collection, EnumSet<Replica.Type> types, boolean onlyLeaders) {

Review Comment:
   I looked at the git history.
   This code comes from a refactor in 2019 of DistributedUpdateProcessor to 
extract the code specific to cloud. At that time this method signature already 
contained the boolean.
   Looking earlier, I saw that the boolean was added in 2018 in a commit “Make 
massive improvements to the tests”. It added the boolean and modified the 
single call to pass true. Well, I believe it was a perf optimization to always 
limit to leaders. At that time the intent was clearly to ignore the dead code, 
and I think the dead code should have been removed from the method instead of 
adding this boolean.
   So after looking at the context, I’m +1 to remove this dead code.



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