You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/02/10 13:33:00 UTC

[GitHub] [ozone] sodonnel opened a new pull request #3071: HDDS-6292. Ensure immutable ContainerReplica set is returned from ContainerStateManagerImpl

sodonnel opened a new pull request #3071:
URL: https://github.com/apache/ozone/pull/3071

## What changes were proposed in this pull request?

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:

```
public Set<ContainerReplica> getContainerReplicas(
final ContainerID containerID) {
Preconditions.checkNotNull(containerID);
final Set<ContainerReplica> replicas = replicaMap.get(containerID);
return replicas == null ? null : Collections.unmodifiableSet(replicas);
}
```

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:

```
Set<> replicas = getContainerReplicas(...)
replicaCount = replicas.size()
// continue to do something based on the size
```

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.

## What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-6292

## How was this patch tested?

Existing tests cover this area.

--
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org