[Hadoop-common-dev] [jira] Created: (HADOOP-5854) findbugs : fix "Inconsistent Synchronization" warnings in hdfs

[ https://issues.apache.org/jira/browse/HADOOP-5854?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Raghu Angadi updated HADOOP-5854:
---------------------------------

Attachment: HADOOP-5854.patch

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.

[ https://issues.apache.org/jira/browse/HADOOP-5854?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Raghu Angadi updated HADOOP-5854:
---------------------------------

Fix Version/s: 0.21.0

[ https://issues.apache.org/jira/browse/HADOOP-5854?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12710011#action_12710011 ]

Suresh Srinivas commented on HADOOP-5854:
-----------------------------------------

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!

[ https://issues.apache.org/jira/browse/HADOOP-5854?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12710032#action_12710032 ]

Raghu Angadi commented on HADOOP-5854:
--------------------------------------

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.

[ https://issues.apache.org/jira/browse/HADOOP-5854?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12710037#action_12710037 ]

Suresh Srinivas commented on HADOOP-5854:
-----------------------------------------

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.

[ https://issues.apache.org/jira/browse/HADOOP-5854?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12710048#action_12710048 ]

Raghu Angadi commented on HADOOP-5854:
--------------------------------------

ant test-patch :

{noformat}
[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]
{noformat}

[ https://issues.apache.org/jira/browse/HADOOP-5854?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12710048#action_12710048 ]

Raghu Angadi edited comment on HADOOP-5854 at 5/15/09 6:29 PM:
---------------------------------------------------------------

Thanks Suresh.

ant test-patch :

{noformat}
[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]
{noformat}

was (Author: rangadi):
ant test-patch :

{noformat}
[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]
{noformat}

[ https://issues.apache.org/jira/browse/HADOOP-5854?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Raghu Angadi resolved HADOOP-5854.
----------------------------------

Resolution: Fixed
Hadoop Flags: [Reviewed]

I just committed this.