adoroszlai commented on code in PR #8990:
URL: https://github.com/apache/ozone/pull/8990#discussion_r2589277905
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/states/ContainerEntry.java:
##########
@@ -31,7 +31,8 @@
*/
public class ContainerEntry {
private final ContainerInfo info;
- private final Map<DatanodeID, ContainerReplica> replicas = new HashMap<>();
+ private Map<ContainerReplica, ContainerReplica> replicas = new HashMap<>();
+ private Set<ContainerReplica> replicaSet;
Review Comment:
`replicaSet` is unused.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/states/ContainerEntry.java:
##########
@@ -42,14 +43,22 @@ public ContainerInfo getInfo() {
}
public Set<ContainerReplica> getReplicas() {
- return new HashSet<>(replicas.values());
+ return Collections.unmodifiableSet(replicas.keySet());
}
public ContainerReplica put(ContainerReplica r) {
- return replicas.put(r.getDatanodeDetails().getID(), r);
+ // Modifications are rare compared to reads, so to optimize for read we
return and unmodifable view of the
+ // map in getReplicas. That means for any modification we need to copy the
map.
+ Map<ContainerReplica, ContainerReplica> map = new HashMap<>(8);
+ map.putAll(this.replicas);
Review Comment:
Please extract to a private function and reuse in `removeReplica`.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/states/ContainerEntry.java:
##########
@@ -42,14 +43,22 @@ public ContainerInfo getInfo() {
}
public Set<ContainerReplica> getReplicas() {
- return new HashSet<>(replicas.values());
+ return Collections.unmodifiableSet(replicas.keySet());
Review Comment:
Since each modification creates a new map, the `unmodifiable` wrapper can be
added on write instead of each read.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/states/ContainerEntry.java:
##########
@@ -42,14 +43,22 @@ public ContainerInfo getInfo() {
}
public Set<ContainerReplica> getReplicas() {
- return new HashSet<>(replicas.values());
+ return Collections.unmodifiableSet(replicas.keySet());
}
public ContainerReplica put(ContainerReplica r) {
- return replicas.put(r.getDatanodeDetails().getID(), r);
+ // Modifications are rare compared to reads, so to optimize for read we
return and unmodifable view of the
+ // map in getReplicas. That means for any modification we need to copy the
map.
+ Map<ContainerReplica, ContainerReplica> map = new HashMap<>(8);
+ map.putAll(this.replicas);
+ replicas = map;
+ return replicas.put(r, r);
}
public ContainerReplica removeReplica(DatanodeID datanodeID) {
+ Map<ContainerReplica, ContainerReplica> map = new HashMap<>(8);
+ map.putAll(this.replicas);
+ replicas = map;
return replicas.remove(datanodeID);
Review Comment:
This doesn't work, since `replicas` has `ContainerReplica` keys, not
`DatanodeID`.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/states/ContainerEntry.java:
##########
@@ -31,7 +31,8 @@
*/
public class ContainerEntry {
private final ContainerInfo info;
- private final Map<DatanodeID, ContainerReplica> replicas = new HashMap<>();
+ private Map<ContainerReplica, ContainerReplica> replicas = new HashMap<>();
Review Comment:
`replicas` can be created lazily by the first modification, until then an
empty map is fine.
```suggestion
private Map<ContainerReplica, ContainerReplica> replicas =
Collections.emptyMap();
```
--
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]
