patsonluk commented on code in PR #2076:
URL: https://github.com/apache/solr/pull/2076#discussion_r1399517132
##########
solr/core/src/java/org/apache/solr/cluster/placement/plugins/OrderedNodePlacementPlugin.java:
##########
@@ -515,6 +521,7 @@ public final void removeReplica(Replica replica) {
});
if (hasReplica.get()) {
removeProjectedReplicaWeights(replica);
+ allReplicas.remove(replica);
Review Comment:
After i read the code again, I'm not 100% about the original purpose of
`hasReplica` flag.
However, it does make the logic a bit more isolated and easier to follow -
iterate the list first, figure out if the replica exists, if it does, deal with
it AFTEr finishing the traversal. In a logical level, it's pretty clean and
avoid any concurrent modification exception (though the map/list being iterated
on is private field anyway).
Hm...I simply move the allReplicas.remove to within the `if
(reps.remove(replica))` block now, cause i do think it's the best if the
operations (add/remove) on the 2 collections stay close to each other.
Anyway, I think these are very minor concerns? 😊 either ways are fine
--
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]
