Hadoop Common
  1. Hadoop Common
  2. HADOOP-5854

findbugs : fix "Inconsistent Synchronization" warnings in hdfs

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      This jira fixes the following findbugs warnings :

      • Inconsistent synchronization of org.apache.hadoop.hdfs.DFSClient$DFSOutputStream.closed; locked 75% of time
      • Inconsistent synchronization of org.apache.hadoop.hdfs.DFSClient$DFSOutputStream.persistBlocks; locked 66% of time
      • Inconsistent synchronization of org.apache.hadoop.hdfs.server.common.UpgradeManager.currentUpgrades; locked 61% of time
      • Inconsistent synchronization of org.apache.hadoop.hdfs.server.common.UpgradeManager.upgradeState; locked 75% of time
      • Inconsistent synchronization of org.apache.hadoop.hdfs.server.namenode.FSDirectory.ready; locked 50% of time
      • Inconsistent synchronization of org.apache.hadoop.hdfs.server.datanode.DataBlockScanner.verificationLog; locked 45% of time
      • Inconsistent synchronization of org.apache.hadoop.hdfs.server.namenode.FSNamesystem.safeMode; locked 48% of time
      • Inconsistent synchronization of org.apache.hadoop.hdfs.server.namenode.FSNamesystem$SafeModeInfo.extension; locked 80% of time
      • Inconsistent synchronization of org.apache.hadoop.io.SequenceFile$Reader.sync; locked 85% of time

        Activity

        Hide
        Raghu Angadi added a comment -

        patch attached. Fixes are of two types :

        • to synchronize properly (or explicitly so that findbugs recognizes)
        • to declare a variable volatile : these warnings are known and are justified in various other jiras. Usually either the current usage is fine or fixing it is not worth the effort.
        Show
        Raghu Angadi added a comment - patch attached. Fixes are of two types : to synchronize properly (or explicitly so that findbugs recognizes) to declare a variable volatile : these warnings are known and are justified in various other jiras. Usually either the current usage is fine or fixing it is not worth the effort.
        Hide
        Suresh Srinivas added a comment -

        This is a good change. Comments:

        1. isInSafeMode() and safemode access with synchronize(this) surrounding it, can be moved out of the synchronized block
        2. FSDirectory.waitForReady() need not have synchronized(this)
        3. Should DFSClient.createBlockOutputStream() where persistBlocks is set to true be synchornized? Not sure how findbugs can ignore this case by just setting the variable to volatile!
        Show
        Suresh Srinivas added a comment - This is a good change. Comments: isInSafeMode() and safemode access with synchronize(this) surrounding it, can be moved out of the synchronized block FSDirectory.waitForReady() need not have synchronized(this) Should DFSClient.createBlockOutputStream() where persistBlocks is set to true be synchornized? Not sure how findbugs can ignore this case by just setting the variable to volatile!
        Hide
        Raghu Angadi added a comment -

        1. not sure
        2. It needs to synchronize since it is going wait on the object. This member is written so that common case (ready is true) does not need any synchronization.
        3. It is under dataQueue lock. findbugs does not detect locks in the callers. But the problem is that other access in fsync() is under a different lock. It can't be fixed easily.

        Show
        Raghu Angadi added a comment - 1. not sure 2. It needs to synchronize since it is going wait on the object. This member is written so that common case (ready is true) does not need any synchronization. 3. It is under dataQueue lock. findbugs does not detect locks in the callers. But the problem is that other access in fsync() is under a different lock. It can't be fixed easily.
        Hide
        Suresh Srinivas added a comment -

        After speaking to Raghu, the code changes makes sense. In case of (3), current change will solve findbugs warnings and will not make other changes needed for correct synchronization. +1.

        Show
        Suresh Srinivas added a comment - After speaking to Raghu, the code changes makes sense. In case of (3), current change will solve findbugs warnings and will not make other changes needed for correct synchronization. +1.
        Hide
        Raghu Angadi added a comment - - edited

        Thanks Suresh.

        ant test-patch :

             [exec]     +1 @author.  The patch does not contain any @author tags.
             [exec]
             [exec]     -1 tests included.  The patch doesn't appear to include any new or modified tests.
             [exec]                         Please justify why no tests are needed for this patch.
             [exec]
             [exec]     +1 javadoc.  The javadoc tool did not generate any warning messages.
             [exec]
             [exec]     +1 javac.  The applied patch does not increase the total number of javac compiler warnings.
             [exec]
             [exec]     +1 findbugs.  The patch does not introduce any new Findbugs warnings.
             [exec]
             [exec]     +1 Eclipse classpath. The patch retains Eclipse classpath integrity.
             [exec]
             [exec]     +1 release audit.  The applied patch does not increase the total number of release audit warnings.
             [exec]
        
        Show
        Raghu Angadi added a comment - - edited Thanks Suresh. ant test-patch : [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] -1 tests included. The patch doesn't appear to include any new or modified tests. [exec] Please justify why no tests are needed for this patch. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. [exec]
        Hide
        Raghu Angadi added a comment -

        I just committed this.

        Show
        Raghu Angadi added a comment - I just committed this.

          People

          • Assignee:
            Raghu Angadi
            Reporter:
            Raghu Angadi
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development