Uploaded image for project: 'Hadoop HDFS'
  1. Hadoop HDFS
  2. HDFS-7261

storageMap is accessed without synchronization in DatanodeDescriptor#updateHeartbeatState()

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.8.0, 3.0.0-alpha1
    • Component/s: None
    • Labels:
      None
    • Target Version/s:

      Description

      Here is the code:

            failedStorageInfos = new HashSet<DatanodeStorageInfo>(
                storageMap.values());
      

      In other places, the lock on "DatanodeDescriptor.storageMap" is held:

          synchronized (storageMap) {
            final Collection<DatanodeStorageInfo> storages = storageMap.values();
            return storages.toArray(new DatanodeStorageInfo[storages.size()]);
          }
      
      1. HDFS-7261.patch
        0.9 kB
        Brahma Reddy Battula
      2. HDFS-7261-001.patch
        2 kB
        Brahma Reddy Battula
      3. HDFS-7261-002.patch
        3 kB
        Brahma Reddy Battula

        Activity

        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk #2099 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2099/)
        HDFS-7261. storageMap is accessed without synchronization in DatanodeDescriptor#updateHeartbeatState() (Brahma Reddy Battula via Colin P. McCabe) (cmccabe: rev 1feb9569f366a29ecb43592d71ee21023162c18f)

        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeDescriptor.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #2099 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2099/ ) HDFS-7261 . storageMap is accessed without synchronization in DatanodeDescriptor#updateHeartbeatState() (Brahma Reddy Battula via Colin P. McCabe) (cmccabe: rev 1feb9569f366a29ecb43592d71ee21023162c18f) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeDescriptor.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #149 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/149/)
        HDFS-7261. storageMap is accessed without synchronization in DatanodeDescriptor#updateHeartbeatState() (Brahma Reddy Battula via Colin P. McCabe) (cmccabe: rev 1feb9569f366a29ecb43592d71ee21023162c18f)

        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeDescriptor.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #149 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/149/ ) HDFS-7261 . storageMap is accessed without synchronization in DatanodeDescriptor#updateHeartbeatState() (Brahma Reddy Battula via Colin P. McCabe) (cmccabe: rev 1feb9569f366a29ecb43592d71ee21023162c18f) hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeDescriptor.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #140 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/140/)
        HDFS-7261. storageMap is accessed without synchronization in DatanodeDescriptor#updateHeartbeatState() (Brahma Reddy Battula via Colin P. McCabe) (cmccabe: rev 1feb9569f366a29ecb43592d71ee21023162c18f)

        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeDescriptor.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #140 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/140/ ) HDFS-7261 . storageMap is accessed without synchronization in DatanodeDescriptor#updateHeartbeatState() (Brahma Reddy Battula via Colin P. McCabe) (cmccabe: rev 1feb9569f366a29ecb43592d71ee21023162c18f) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeDescriptor.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk #2081 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2081/)
        HDFS-7261. storageMap is accessed without synchronization in DatanodeDescriptor#updateHeartbeatState() (Brahma Reddy Battula via Colin P. McCabe) (cmccabe: rev 1feb9569f366a29ecb43592d71ee21023162c18f)

        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeDescriptor.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #2081 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2081/ ) HDFS-7261 . storageMap is accessed without synchronization in DatanodeDescriptor#updateHeartbeatState() (Brahma Reddy Battula via Colin P. McCabe) (cmccabe: rev 1feb9569f366a29ecb43592d71ee21023162c18f) hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeDescriptor.java
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Hadoop-Yarn-trunk #883 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/883/)
        HDFS-7261. storageMap is accessed without synchronization in DatanodeDescriptor#updateHeartbeatState() (Brahma Reddy Battula via Colin P. McCabe) (cmccabe: rev 1feb9569f366a29ecb43592d71ee21023162c18f)

        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeDescriptor.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Yarn-trunk #883 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/883/ ) HDFS-7261 . storageMap is accessed without synchronization in DatanodeDescriptor#updateHeartbeatState() (Brahma Reddy Battula via Colin P. McCabe) (cmccabe: rev 1feb9569f366a29ecb43592d71ee21023162c18f) hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeDescriptor.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #149 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/149/)
        HDFS-7261. storageMap is accessed without synchronization in DatanodeDescriptor#updateHeartbeatState() (Brahma Reddy Battula via Colin P. McCabe) (cmccabe: rev 1feb9569f366a29ecb43592d71ee21023162c18f)

        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeDescriptor.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #149 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/149/ ) HDFS-7261 . storageMap is accessed without synchronization in DatanodeDescriptor#updateHeartbeatState() (Brahma Reddy Battula via Colin P. McCabe) (cmccabe: rev 1feb9569f366a29ecb43592d71ee21023162c18f) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeDescriptor.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        brahmareddy Brahma Reddy Battula added a comment -

        Thanks a lot Colin P. McCabe

        Show
        brahmareddy Brahma Reddy Battula added a comment - Thanks a lot Colin P. McCabe
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-trunk-Commit #7463 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7463/)
        HDFS-7261. storageMap is accessed without synchronization in DatanodeDescriptor#updateHeartbeatState() (Brahma Reddy Battula via Colin P. McCabe) (cmccabe: rev 1feb9569f366a29ecb43592d71ee21023162c18f)

        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeDescriptor.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #7463 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7463/ ) HDFS-7261 . storageMap is accessed without synchronization in DatanodeDescriptor#updateHeartbeatState() (Brahma Reddy Battula via Colin P. McCabe) (cmccabe: rev 1feb9569f366a29ecb43592d71ee21023162c18f) hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeDescriptor.java
        Hide
        cmccabe Colin P. McCabe added a comment -

        +1. Thanks, Brahma Reddy Battula.

        Show
        cmccabe Colin P. McCabe added a comment - +1. Thanks, Brahma Reddy Battula .
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12705313/HDFS-7261-002.patch
        against trunk revision 3411732.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9957//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9957//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12705313/HDFS-7261-002.patch against trunk revision 3411732. +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9957//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9957//console This message is automatically generated.
        Hide
        brahmareddy Brahma Reddy Battula added a comment -

        Thanks a lot for review.. will update the patch..

        Show
        brahmareddy Brahma Reddy Battula added a comment - Thanks a lot for review.. will update the patch..
        Hide
        cmccabe Colin P. McCabe added a comment -

        thanks, Brahma Reddy Battula.

        403 synchronized (storageMap)

        { 404 failedStorageInfos = new HashSet<DatanodeStorageInfo>( 404 storageMap.values()); 405 storageMap.values()); 406 }

        the indentation is off on line 404

        similarly, the indentation is off in pruneStorageMap

        Show
        cmccabe Colin P. McCabe added a comment - thanks, Brahma Reddy Battula . 403 synchronized (storageMap) { 404 failedStorageInfos = new HashSet<DatanodeStorageInfo>( 404 storageMap.values()); 405 storageMap.values()); 406 } the indentation is off on line 404 similarly, the indentation is off in pruneStorageMap
        Hide
        brahmareddy Brahma Reddy Battula added a comment -

        Colin P. McCabe Kindly review the patch and can you please kick jenkin once..?

        Show
        brahmareddy Brahma Reddy Battula added a comment - Colin P. McCabe Kindly review the patch and can you please kick jenkin once..?
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12703042/HDFS-7261-001.patch
        against trunk revision 24db081.

        -1 patch. Trunk compilation may be broken.

        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9782//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12703042/HDFS-7261-001.patch against trunk revision 24db081. -1 patch . Trunk compilation may be broken. Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9782//console This message is automatically generated.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12703042/HDFS-7261-001.patch
        against trunk revision 95bfd08.

        -1 patch. Trunk compilation may be broken.

        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9774//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12703042/HDFS-7261-001.patch against trunk revision 95bfd08. -1 patch . Trunk compilation may be broken. Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9774//console This message is automatically generated.
        Hide
        brahmareddy Brahma Reddy Battula added a comment -

        Colin P. McCabe Thanks a lot for looking into this issue...Yes, I missed above two..Updated the patch,Kindly review..

        Show
        brahmareddy Brahma Reddy Battula added a comment - Colin P. McCabe Thanks a lot for looking into this issue...Yes, I missed above two..Updated the patch,Kindly review..
        Hide
        cmccabe Colin P. McCabe added a comment -

        Thanks, Brahma.

        It looks like storageMap is also being accessed without synchronization here:

            if (storageMap.size() != reports.length) {
              pruneStorageMap(reports);
            }
         

        and here:

          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<String, DatanodeStorageInfo> excessStorages;
        
            synchronized (storageMap) {
              // Init excessStorages with all known storages.
        

        We should be able to avoid the lack of synchronization, and avoid holding the lock for a long time, by getting the size() before we release the lock.

        Show
        cmccabe Colin P. McCabe added a comment - Thanks, Brahma. It looks like storageMap is also being accessed without synchronization here: if (storageMap.size() != reports.length) { pruneStorageMap(reports); } and here: 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< String , DatanodeStorageInfo> excessStorages; synchronized (storageMap) { // Init excessStorages with all known storages. We should be able to avoid the lack of synchronization, and avoid holding the lock for a long time, by getting the size() before we release the lock.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12702819/HDFS-7261.patch
        against trunk revision 1b67209.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

        org.apache.hadoop.hdfs.TestLeaseRecovery2
        org.apache.hadoop.hdfs.server.namenode.ha.TestPipelinesFailover
        org.apache.hadoop.hdfs.server.blockmanagement.TestDatanodeManager

        The following test timeouts occurred in hadoop-hdfs-project/hadoop-hdfs:

        org.apache.hadoop.hdfs.TestAppendSnapshotTruncate

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9750//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9750//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12702819/HDFS-7261.patch against trunk revision 1b67209. +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestLeaseRecovery2 org.apache.hadoop.hdfs.server.namenode.ha.TestPipelinesFailover org.apache.hadoop.hdfs.server.blockmanagement.TestDatanodeManager The following test timeouts occurred in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestAppendSnapshotTruncate Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9750//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9750//console This message is automatically generated.

          People

          • Assignee:
            brahmareddy Brahma Reddy Battula
            Reporter:
            yuzhihong@gmail.com Ted Yu
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development