diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt index d4baaf3f83a..b3cc6b779cf 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt @@ -64,6 +64,10 @@ Release 2.8.0 - UNRELEASED HDFS-8002. Website refers to /trash directory. (Brahma Reddy Battula via aajisaka) + HDFS-7261. storageMap is accessed without synchronization in + DatanodeDescriptor#updateHeartbeatState() (Brahma Reddy Battula via Colin + P. McCabe) + Release 2.7.0 - UNRELEASED INCOMPATIBLE CHANGES diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeDescriptor.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeDescriptor.java index 96084a43005..1ab2ab91905 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeDescriptor.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeDescriptor.java @@ -449,8 +449,10 @@ public class DatanodeDescriptor extends DatanodeInfo { if (checkFailedStorages) { LOG.info("Number of failed storage changes from " + this.volumeFailures + " to " + volFailures); - failedStorageInfos = new HashSet( - storageMap.values()); + synchronized (storageMap) { + failedStorageInfos = + new HashSet(storageMap.values()); + } } setCacheCapacity(cacheCapacity); @@ -482,8 +484,11 @@ public class DatanodeDescriptor extends DatanodeInfo { if (checkFailedStorages) { updateFailedStorage(failedStorageInfos); } - - if (storageMap.size() != reports.length) { + long storageMapSize; + synchronized (storageMap) { + storageMapSize = storageMap.size(); + } + if (storageMapSize != reports.length) { pruneStorageMap(reports); } } @@ -493,14 +498,14 @@ public class DatanodeDescriptor extends DatanodeInfo { * as long as they have associated block replicas. */ private void pruneStorageMap(final StorageReport[] reports) { - if (LOG.isDebugEnabled()) { - LOG.debug("Number of storages reported in heartbeat=" + reports.length + - "; Number of storages in storageMap=" + storageMap.size()); - } - - HashMap excessStorages; - synchronized (storageMap) { + if (LOG.isDebugEnabled()) { + LOG.debug("Number of storages reported in heartbeat=" + reports.length + + "; Number of storages in storageMap=" + storageMap.size()); + } + + HashMap excessStorages; + // Init excessStorages with all known storages. excessStorages = new HashMap(storageMap); @@ -517,8 +522,8 @@ public class DatanodeDescriptor extends DatanodeInfo { LOG.info("Removed storage " + storageInfo + " from DataNode" + this); } else if (LOG.isDebugEnabled()) { // This can occur until all block reports are received. - LOG.debug("Deferring removal of stale storage " + storageInfo + - " with " + storageInfo.numBlocks() + " blocks"); + LOG.debug("Deferring removal of stale storage " + storageInfo + + " with " + storageInfo.numBlocks() + " blocks"); } } }