Montura commented on PR #6303: URL: https://github.com/apache/ozone/pull/6303#issuecomment-1977415944
> Since there are multiple nodes, removing `synchronized` can lead to an inconsistent state. Suppose Pipeline `p` has datanodes [A, B, C] and we have the following two calls: > > 1. addPipeline(p) > 2. removePipeline(p) > > With `synchronized`, all datanodes A, B, C will be added and then removed. > > Without `synchronized`, it may > > * add A, B > * remove A, B, C > * add C > > Then, C is not removed. I assume that everything will be ok: 1. [`SCMNodeManager::addPipeline`](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java#L1399) has 2 usages: a. [`private PipelineStateManagerImpl::initialize`](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateManagerImpl.java#L87) used in `PipelineStateManagerImpl::constructor` b. [`public PipelineStateManagerImpl::addPipeline`](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateManagerImpl.java#L100) 2. [`public SCMNodeManager::removePipeline`](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java#L1409) has a single usage in [`PipelineStateManagerImpl::removePipeline`](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateManagerImpl.java#L242) So, except the `PipelineStateManagerImpl::constructor`, methods `SCMNodeManager::addPipeline` and `SCMNodeManager::removePipeline` are called under *write_lock* from `PipelineStateManagerImpl`, then `Node2PipelineMap::addPipeline` and `Node2PipelineMap::removePipeline` could be `non-synchroinized`: * [Acquire *WriteLock* in `PipelineStateManagerImpl::addPipeline`](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateManagerImpl.java#L96) -> [`PipelineStateManagerImpl::addPipeline`](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateManagerImpl.java#L100) -> [`SCMNodeManager::addPipeline`](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java#L1399) -> [`NodeStateManager::addPipeline`](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeStateManager.java#L357) -> `Node2PipelineMap::addPipeline` -> [Release *WriteLock* in `PipelineStateManagerImpl::addPipeline`](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateManagerImpl.java#L105) * [Acquire *WriteLock* in `PipelineStateManagerImpl::removePipeline`](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateManagerImpl.java#L239) -> [`PipelineStateManagerImpl::removePipeline`](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateManagerImpl.java#L242) -> [`SCMNodeManager::removePipeline`](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java#L1409) -> [`NodeStateManager::removePipeline`](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeStateManager.java#L630) -> `Node2PipelineMap::removePipeline` -> [Release *WriteLock* in `PipelineStateManagerImpl::removePipeline`](https://github.com/apache/ozone/blob/master/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateManagerImpl.java#L247) @szetszwo, maybe you know any more about this usages or some extra scenarios, then my thoughts are wrong ... -- 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]
