SOLR-17054: Remove unused and duplicate code in DistributedZkUpdateProcessor - #2038
Conversation
| useNodes = nodes; | ||
| params.set(DISTRIB_UPDATE_PARAM, DistribPhase.TOLEADER.toString()); | ||
| params.set(COMMIT_END_POINT, "leaders"); | ||
| if (useNodes != null) { |
| // that means I want to forward onto my replicas... | ||
| // so get the replicas... | ||
| forwardToLeader = false; | ||
| String leaderCoreNodeName = leaderReplica.getName(); |
There was a problem hiding this comment.
Was exactly the same as getReplicaNodesForLeader, except for the timeout (has been added as an argument of the method).
| } | ||
|
|
||
| private List<SolrCmdDistributor.Node> getCollectionUrls( | ||
| String collection, EnumSet<Replica.Type> types, boolean onlyLeaders) { |
There was a problem hiding this comment.
onlyLeaders was always true therefore types was not used.
There was a problem hiding this 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.
| } | ||
|
|
||
| private List<SolrCmdDistributor.Node> getCollectionUrls( | ||
| String collection, EnumSet<Replica.Type> types, boolean onlyLeaders) { |
There was a problem hiding this 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.
|
Could you create a JIRA issue for this PR to give it more visibility? |
pvcnt
changed the title
|
@bruno-roustant JIRA ticket has been created: https://issues.apache.org/jira/browse/SOLR-17054 |
|
Thanks Vincent, let's merge it! |
…ocessor (#2038) Co-authored-by: Vincent Primault <vprimault@salesforce.com>
|
Nice clean up! |
3 participants
https://issues.apache.org/jira/browse/SOLR-17054
Description
There is dead code and duplicate code in DistributedZkUpdateProcessor. It makes understanding this already complex class more difficult.
Solution
Dead code and duplicate code has been removed, using automated refactoring tools available in Intellij whenever possible, to reduce the risk of errors.
Tests
No new code.
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.