[
https://issues.apache.org/jira/browse/HDDS-6292?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Stephen O'Donnell updated HDDS-6292:
------------------------------------
Description:
Inside ContainerStateMap, the replicas for a container are stored in a Set
backed by a ConcurrentHashMap.
When you ask for the current replicas of a container, this method is used:
{code:java}
public Set<ContainerReplica> getContainerReplicas(
final ContainerID containerID) {
Preconditions.checkNotNull(containerID);
final Set<ContainerReplica> replicas = replicaMap.get(containerID);
return replicas == null ? null : Collections.unmodifiableSet(replicas);
} {code}
Note that it pulls out the Set, wraps it as unmodifiable and returns it.
There is a problem here, in that if the Set is updated by ICR / FCR at the same
time as another part of the code has taken a reference to it, the other part of
the code can make incorrect decisions. Eg:
{code:java}
Set<> replicas = getContainerReplicas(...)
replicaCount = replicas.size()
// continue to do something based on the size{code}
ReplicationManger has run into a race condition like this. We also use the
Replicas to form pipelines for closed containers, so I worry there could be
some strange issues if the set if mutated during the pipeline creation.
I see two possible solutions here. `GetContainerReplicas` should create a copy
of the Set and return that, so the copy the other part of the code gets is its
own copy and nothing can change it.
Or, we make the Set immutable, so that each new replica details are received,
we create the new copy of the set and store that. Then any other parts of the
code can get a reference to it, and know it will never change.
Mutations to the replicas for a closed container will only happen with FCR,
which is relatively rare.
However we may ask for read pipelines very frequently, so it would be cheaper
overall to use option 2.
It we go with option 2, I think we can move from a concurrentHashMap to a plain
hashMap too, which may make the memory footprint slightly smaller.
Note access to the replicas is via ContainerStateManagerImpl, which already has
a course RW lock protecting access to the container manager. Quite possibly FCR
reporting could be improved by a finer grained or striped lock.
This problem was reported in HDDS-5643.
was:
Inside ContainerStateMap, the replicas for a container are stored in a Set
backed by a ConcurrentHashMap.
When you ask for the current replicas of a container, this method is used:
{code:java}
public Set<ContainerReplica> getContainerReplicas(
final ContainerID containerID) {
Preconditions.checkNotNull(containerID);
final Set<ContainerReplica> replicas = replicaMap.get(containerID);
return replicas == null ? null : Collections.unmodifiableSet(replicas);
} {code}
Note that it pulls out the Set, wraps it as unmodifiable and returns it.
There is a problem here, in that if the Set is updated by ICR / FCR at the same
time as another part of the code has taken a reference to it, the other part of
the code can make incorrect decisions. Eg:
{code:java}
Set<> replicas = getContainerReplicas(...)
replicaCount = replicas.size()
// continue to do something based on the size{code}
ReplicationManger has run into a race condition like this. We also use the
Replicas to form pipelines for closed containers, so I worry there could be
some strange issues if the set if mutated during the pipeline creation.
I see two possible solutions here. `GetContainerReplicas` should create a copy
of the Set and return that, so the copy the other part of the code gets is its
own copy and nothing can change it.
Or, we make the Set immutable, so that each new replica details are received,
we create the new copy of the set and store that. Then any other parts of the
code can get a reference to it, and know it will never change.
Mutations to the replicas for a closed container will only happen with FCR,
which is relatively rare.
However we may ask for read pipelines very frequently, so it would be cheaper
overall to use option 2.
It we go with option 2, I think we can move from a concurrentHashMap to a plain
hashMap too, which may make the memory footprint slightly smaller.
Note access to the replicas is via ContainerStateManagerImpl, which already has
a course RW lock protecting access to the container manager. Quite possibly FCR
reporting could be improved by a finer grained or striped lock.
> Ensure immutable ContainerReplica set is returned from
> ContainerStateManagerImpl
> --------------------------------------------------------------------------------
>
> Key: HDDS-6292
> URL: https://issues.apache.org/jira/browse/HDDS-6292
> Project: Apache Ozone
> Issue Type: Improvement
> Components: SCM
> Reporter: Stephen O'Donnell
> Assignee: Stephen O'Donnell
> Priority: Major
>
> Inside ContainerStateMap, the replicas for a container are stored in a Set
> backed by a ConcurrentHashMap.
> When you ask for the current replicas of a container, this method is used:
> {code:java}
> public Set<ContainerReplica> getContainerReplicas(
> final ContainerID containerID) {
> Preconditions.checkNotNull(containerID);
> final Set<ContainerReplica> replicas = replicaMap.get(containerID);
> return replicas == null ? null : Collections.unmodifiableSet(replicas);
> } {code}
> Note that it pulls out the Set, wraps it as unmodifiable and returns it.
> There is a problem here, in that if the Set is updated by ICR / FCR at the
> same time as another part of the code has taken a reference to it, the other
> part of the code can make incorrect decisions. Eg:
>
> {code:java}
> Set<> replicas = getContainerReplicas(...)
> replicaCount = replicas.size()
> // continue to do something based on the size{code}
> ReplicationManger has run into a race condition like this. We also use the
> Replicas to form pipelines for closed containers, so I worry there could be
> some strange issues if the set if mutated during the pipeline creation.
> I see two possible solutions here. `GetContainerReplicas` should create a
> copy of the Set and return that, so the copy the other part of the code gets
> is its own copy and nothing can change it.
> Or, we make the Set immutable, so that each new replica details are received,
> we create the new copy of the set and store that. Then any other parts of the
> code can get a reference to it, and know it will never change.
> Mutations to the replicas for a closed container will only happen with FCR,
> which is relatively rare.
> However we may ask for read pipelines very frequently, so it would be cheaper
> overall to use option 2.
> It we go with option 2, I think we can move from a concurrentHashMap to a
> plain hashMap too, which may make the memory footprint slightly smaller.
> Note access to the replicas is via ContainerStateManagerImpl, which already
> has a course RW lock protecting access to the container manager. Quite
> possibly FCR reporting could be improved by a finer grained or striped lock.
> This problem was reported in HDDS-5643.
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
