patsonluk commented on code in PR #2076:
URL: https://github.com/apache/solr/pull/2076#discussion_r1394983063
##########
solr/core/src/java/org/apache/solr/cluster/placement/plugins/OrderedNodePlacementPlugin.java:
##########
@@ -404,20 +404,25 @@ public abstract static class WeightedNode implements
Comparable<WeightedNode> {
private final Node node;
private final Map<String, Map<String, Set<Replica>>> replicas;
+ // a flattened list of all replicas, as computing from the map could be
costly
+ private final Set<Replica> allReplicas;
+
public WeightedNode(Node node) {
this.node = node;
this.replicas = new HashMap<>();
+ this.allReplicas = new HashSet<>();
}
public Node getNode() {
return node;
}
public Set<Replica> getAllReplicasOnNode() {
- return replicas.values().stream()
- .flatMap(shard -> shard.values().stream())
- .flatMap(Collection::stream)
- .collect(Collectors.toSet());
+ return new HashSet<>(allReplicas);
Review Comment:
As discussed, the current fix is to use the most defensive approach which
does not modify the behavior at all. The minor concern with `unmodifableSet`
(which I did use at first) was that the "view" could still mutate after this
method returns and this could break certain expectation of the caller.
Otherwise I don't have any objection using `unmodifableSet`. Perhaps
@HoustonPutman can share some thoughts? 😊
##########
solr/core/src/java/org/apache/solr/cluster/placement/plugins/OrderedNodePlacementPlugin.java:
##########
@@ -404,20 +404,25 @@ public abstract static class WeightedNode implements
Comparable<WeightedNode> {
private final Node node;
private final Map<String, Map<String, Set<Replica>>> replicas;
+ // a flattened list of all replicas, as computing from the map could be
costly
+ private final Set<Replica> allReplicas;
+
public WeightedNode(Node node) {
this.node = node;
this.replicas = new HashMap<>();
+ this.allReplicas = new HashSet<>();
}
public Node getNode() {
return node;
}
public Set<Replica> getAllReplicasOnNode() {
- return replicas.values().stream()
- .flatMap(shard -> shard.values().stream())
- .flatMap(Collection::stream)
- .collect(Collectors.toSet());
+ return new HashSet<>(allReplicas);
Review Comment:
As discussed, the current fix is to use the most defensive approach which
does not modify the behavior at all. The minor concern with `unmodifableSet`
was that the "view" could still mutate after this method returns and this could
break certain expectation of the caller.
Otherwise I don't have any objection using `unmodifableSet`. Perhaps
@HoustonPutman can share some thoughts? 😊
--
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]
