Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.23.0
    • Component/s: namenode
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    1. h2191_20110723.patch
      48 kB
      Tsz Wo Nicholas Sze
    2. h2191_20110723b.patch
      49 kB
      Tsz Wo Nicholas Sze
    3. h2191_20110726.patch
      58 kB
      Tsz Wo Nicholas Sze

      Issue Links

        Activity

        Hide
        Tsz Wo Nicholas Sze added a comment -

        Hi Todd,

        Oops, I have accidentally changed it. Created HDFS-2706. Thanks.

        Show
        Tsz Wo Nicholas Sze added a comment - Hi Todd, Oops, I have accidentally changed it. Created HDFS-2706 . Thanks.
        Hide
        Todd Lipcon added a comment -

        Hi Nicholas. After this patch, the block invalidate limit is no longer configurable. Was this intentional? We still have the config key in DFSConfigKeys, but it's unused.

        Show
        Todd Lipcon added a comment - Hi Nicholas. After this patch, the block invalidate limit is no longer configurable. Was this intentional? We still have the config key in DFSConfigKeys, but it's unused.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #737 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/737/)
        HDFS-2191. Move datanodeMap from FSNamesystem to DatanodeManager.

        szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1151339
        Files :

        • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/blockmanagement/TestHeartbeatHandling.java
        • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/NameNodeAdapter.java
        • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestComputeInvalidateWork.java
        • /hadoop/common/trunk/hdfs/CHANGES.txt
        • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/datanode/TestBlockReport.java
        • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/TestDecommission.java
        • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java
        • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/blockmanagement/DecommissionManager.java
        • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
        • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestDeadDatanode.java
        • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/blockmanagement/BlockManagerTestUtil.java
        • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/blockmanagement/TestComputeInvalidateWork.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #737 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/737/ ) HDFS-2191 . Move datanodeMap from FSNamesystem to DatanodeManager. szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1151339 Files : /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/blockmanagement/TestHeartbeatHandling.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/NameNodeAdapter.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestComputeInvalidateWork.java /hadoop/common/trunk/hdfs/CHANGES.txt /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/datanode/TestBlockReport.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/TestDecommission.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/blockmanagement/DecommissionManager.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestDeadDatanode.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/blockmanagement/BlockManagerTestUtil.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/blockmanagement/TestComputeInvalidateWork.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk-Commit #807 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/807/)
        HDFS-2191. Move datanodeMap from FSNamesystem to DatanodeManager.

        szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1151339
        Files :

        • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/blockmanagement/TestHeartbeatHandling.java
        • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/NameNodeAdapter.java
        • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestComputeInvalidateWork.java
        • /hadoop/common/trunk/hdfs/CHANGES.txt
        • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/datanode/TestBlockReport.java
        • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/TestDecommission.java
        • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java
        • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/blockmanagement/DecommissionManager.java
        • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
        • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestDeadDatanode.java
        • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/blockmanagement/BlockManagerTestUtil.java
        • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/blockmanagement/TestComputeInvalidateWork.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #807 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/807/ ) HDFS-2191 . Move datanodeMap from FSNamesystem to DatanodeManager. szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1151339 Files : /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/blockmanagement/TestHeartbeatHandling.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/NameNodeAdapter.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestComputeInvalidateWork.java /hadoop/common/trunk/hdfs/CHANGES.txt /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/datanode/TestBlockReport.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/TestDecommission.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/blockmanagement/DecommissionManager.java /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestDeadDatanode.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/blockmanagement/BlockManagerTestUtil.java /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/blockmanagement/TestComputeInvalidateWork.java
        Hide
        Tsz Wo Nicholas Sze added a comment -

        I have committed this.

        Show
        Tsz Wo Nicholas Sze added a comment - I have committed this.
        Hide
        Suresh Srinivas added a comment -

        > heartbeatIntervalSeconds uses DFSConfigKeys.DFS_HEARTBEAT_INTERVAL_KEY and the default is 3 (seconds).
        You are correct. I was confused.

        +1 for the patch.

        Show
        Suresh Srinivas added a comment - > heartbeatIntervalSeconds uses DFSConfigKeys.DFS_HEARTBEAT_INTERVAL_KEY and the default is 3 (seconds). You are correct. I was confused. +1 for the patch.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        heartbeatIntervalSeconds is actually in milliseconds. So can you call it heartbeatIntervalMilliSeconds. Rest of the code looks correct.

        heartbeatIntervalSeconds uses DFSConfigKeys.DFS_HEARTBEAT_INTERVAL_KEY and the default is 3 (seconds).

        DatanodeManager#getDatanode() - how is the map access synchronized in this?
        > It should accquire the write lock of DatanodeManager when we have read-write lock in DatanodeManager in the future.
        We should track this with a jira, since the synchronization that is in the current code is not correct.

        I went through the patch. It did not change any existing synchronization on datanodesMap. However, the access of DatanodeManager#getDatanode(..) is sometimes not synchronized. You are right that the current synchronization is incorrect.

        I have created HDFS-2206 for adding read-write lock to DatanodeManager.

        Show
        Tsz Wo Nicholas Sze added a comment - heartbeatIntervalSeconds is actually in milliseconds. So can you call it heartbeatIntervalMilliSeconds. Rest of the code looks correct. heartbeatIntervalSeconds uses DFSConfigKeys.DFS_HEARTBEAT_INTERVAL_KEY and the default is 3 (seconds). DatanodeManager#getDatanode() - how is the map access synchronized in this? > It should accquire the write lock of DatanodeManager when we have read-write lock in DatanodeManager in the future. We should track this with a jira, since the synchronization that is in the current code is not correct. I went through the patch. It did not change any existing synchronization on datanodesMap. However, the access of DatanodeManager#getDatanode(..) is sometimes not synchronized. You are right that the current synchronization is incorrect. I have created HDFS-2206 for adding read-write lock to DatanodeManager.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12487798/h2191_20110726.patch
        against trunk revision 1150960.

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

        +1 tests included. The patch appears to include 24 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

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

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

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

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        +1 system test framework. The patch passed system test framework compile.

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/1020//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/1020//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1020//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12487798/h2191_20110726.patch against trunk revision 1150960. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 24 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/1020//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/1020//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1020//console This message is automatically generated.
        Hide
        Suresh Srinivas added a comment -

        > Is this correct? 10000 * heartbeatIntervalSeconds Why 10000? Should you be multiplying it by 1000 when getting it from conf and not 10000 *?
        Sorry I was not clear in my comments. The newly added code is:

        +    
        +    final long heartbeatIntervalSeconds = conf.getLong(
        +        DFSConfigKeys.DFS_HEARTBEAT_INTERVAL_KEY,
        +        DFSConfigKeys.DFS_HEARTBEAT_INTERVAL_DEFAULT);
        +    final int heartbeatRecheckInterval = conf.getInt(
        +        DFSConfigKeys.DFS_NAMENODE_HEARTBEAT_RECHECK_INTERVAL_KEY, 
        +        DFSConfigKeys.DFS_NAMENODE_HEARTBEAT_RECHECK_INTERVAL_DEFAULT); // 5 minutes
        +    this.heartbeatExpireInterval = 2 * heartbeatRecheckInterval
        +        + 10000 * heartbeatIntervalSeconds;
        

        heartbeatIntervalSeconds is actually in milliseconds. So can you call it heartbeatIntervalMilliSeconds. Rest of the code looks correct.

        > FSNamesystem currently requires quite a few methods in DatanodeMnaager, e.g. getNetworkTopology(), registerDatanode(..), etc. The getDatanode(..) methods are only two of those methods. Let's think about how to change the APIs after the code separation is done. Okay?
        Sounds good

        > I think you are talking about FSNamesystem.heartbeatCheck(). I incorrectly removed the synchronization. Fixed it.
        DatanodeManager#getDatanode() - how is the map access synchronized in this?

        > It should accquire the write lock of DatanodeManager when we have read-write lock in DatanodeManager in the future.
        We should track this with a jira, since the synchronization that is in the current code is not correct.

        Show
        Suresh Srinivas added a comment - > Is this correct? 10000 * heartbeatIntervalSeconds Why 10000? Should you be multiplying it by 1000 when getting it from conf and not 10000 *? Sorry I was not clear in my comments. The newly added code is: + + final long heartbeatIntervalSeconds = conf.getLong( + DFSConfigKeys.DFS_HEARTBEAT_INTERVAL_KEY, + DFSConfigKeys.DFS_HEARTBEAT_INTERVAL_DEFAULT); + final int heartbeatRecheckInterval = conf.getInt( + DFSConfigKeys.DFS_NAMENODE_HEARTBEAT_RECHECK_INTERVAL_KEY, + DFSConfigKeys.DFS_NAMENODE_HEARTBEAT_RECHECK_INTERVAL_DEFAULT); // 5 minutes + this.heartbeatExpireInterval = 2 * heartbeatRecheckInterval + + 10000 * heartbeatIntervalSeconds; heartbeatIntervalSeconds is actually in milliseconds. So can you call it heartbeatIntervalMilliSeconds . Rest of the code looks correct. > FSNamesystem currently requires quite a few methods in DatanodeMnaager, e.g. getNetworkTopology(), registerDatanode(..), etc. The getDatanode(..) methods are only two of those methods. Let's think about how to change the APIs after the code separation is done. Okay? Sounds good > I think you are talking about FSNamesystem.heartbeatCheck(). I incorrectly removed the synchronization. Fixed it. DatanodeManager#getDatanode() - how is the map access synchronized in this? > It should accquire the write lock of DatanodeManager when we have read-write lock in DatanodeManager in the future. We should track this with a jira, since the synchronization that is in the current code is not correct.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12487798/h2191_20110726.patch
        against trunk revision 1150960.

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

        +1 tests included. The patch appears to include 24 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

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

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

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

        -1 core tests. The patch failed these core unit tests:

        +1 contrib tests. The patch passed contrib unit tests.

        +1 system test framework. The patch passed system test framework compile.

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/1012//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/1012//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1012//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12487798/h2191_20110726.patch against trunk revision 1150960. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 24 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/1012//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/1012//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1012//console This message is automatically generated.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        Thanks Suresh for the comments.

        h2191_20110726.patch: addressed everything except for #2.

        1. Is this correct? 10000 * heartbeatIntervalSeconds Why 10000? Should you be multiplying it by 1000 when getting it from conf and not 10000 *?

        It is correct. In the original code, we have

        -    long heartbeatInterval = conf.getLong(
        -        DFSConfigKeys.DFS_HEARTBEAT_INTERVAL_KEY,
        -        DFSConfigKeys.DFS_HEARTBEAT_INTERVAL_DEFAULT) * 1000;
             this.heartbeatRecheckInterval = conf.getInt(
                 DFSConfigKeys.DFS_NAMENODE_HEARTBEAT_RECHECK_INTERVAL_KEY, 
                 DFSConfigKeys.DFS_NAMENODE_HEARTBEAT_RECHECK_INTERVAL_DEFAULT); // 5 minutes
        -    this.heartbeatExpireInterval = 2 * heartbeatRecheckInterval +
        -      10 * heartbeatInterval;
        

        So, 10000 = 10*1000 is correct. I changed heartbeatInterval (in ms) to heartbeatInterval to heartbeatIntervalSeconds since we will convert heartbeatInterval back to seconds in calculating blockInvalidateLimit below.

        -    this.blockInvalidateLimit = Math.max(this.blockInvalidateLimit, 
        -                                         20*(int)(heartbeatInterval/1000));
        

        2. blockManager.getDatanodeManager().getDatanode() - we could add a method blockManager.getDataNode() - I prefer this instead of exposing DatanodeManager to FSNamesystem.

        FSNamesystem currently requires quite a few methods in DatanodeMnaager, e.g. getNetworkTopology(), registerDatanode(..), etc. The getDatanode(..) methods are only two of those methods. Let's think about how to change the APIs after the code separation is done. Okay?

        3. Earlier getDatanode() used to lock using datanodeMap, do we need that with your patch? How is it synchronized now? Also datanodeMap access may not be synchronized correctly - for example datanodeDump synchronizes it but other instances such as DatanodeManager#getDatanode() does not.

        I think you are talking about FSNamesystem.heartbeatCheck(). I incorrectly removed the synchronization. Fixed it.

        4. In a future patch FSNamesystem#heartbeats and its synchronization should move to blockmanager

        That's correct.

        5. Please change javadoc of getDatanode() as throws UnregisteredNodeException.

        Done.

        6. I am sure you have already planned for removing FSNamesystem#blockInvalidateLimit in future patch.

        Removed.

        7. How is DatanodeManager#getDatanodeCyclicIteration() synchronized? It cannot be synchronized using FSNamesystem.writeLock() as done currently?

        It should accquire the write lock of DatanodeManager when we have read-write lock in DatanodeManager in the future.

        Show
        Tsz Wo Nicholas Sze added a comment - Thanks Suresh for the comments. h2191_20110726.patch: addressed everything except for #2. 1. Is this correct? 10000 * heartbeatIntervalSeconds Why 10000? Should you be multiplying it by 1000 when getting it from conf and not 10000 *? It is correct. In the original code, we have - long heartbeatInterval = conf.getLong( - DFSConfigKeys.DFS_HEARTBEAT_INTERVAL_KEY, - DFSConfigKeys.DFS_HEARTBEAT_INTERVAL_DEFAULT) * 1000; this .heartbeatRecheckInterval = conf.getInt( DFSConfigKeys.DFS_NAMENODE_HEARTBEAT_RECHECK_INTERVAL_KEY, DFSConfigKeys.DFS_NAMENODE_HEARTBEAT_RECHECK_INTERVAL_DEFAULT); // 5 minutes - this .heartbeatExpireInterval = 2 * heartbeatRecheckInterval + - 10 * heartbeatInterval; So, 10000 = 10*1000 is correct. I changed heartbeatInterval (in ms) to heartbeatInterval to heartbeatIntervalSeconds since we will convert heartbeatInterval back to seconds in calculating blockInvalidateLimit below. - this .blockInvalidateLimit = Math .max( this .blockInvalidateLimit, - 20*( int )(heartbeatInterval/1000)); 2. blockManager.getDatanodeManager().getDatanode() - we could add a method blockManager.getDataNode() - I prefer this instead of exposing DatanodeManager to FSNamesystem. FSNamesystem currently requires quite a few methods in DatanodeMnaager, e.g. getNetworkTopology(), registerDatanode(..), etc. The getDatanode(..) methods are only two of those methods. Let's think about how to change the APIs after the code separation is done. Okay? 3. Earlier getDatanode() used to lock using datanodeMap, do we need that with your patch? How is it synchronized now? Also datanodeMap access may not be synchronized correctly - for example datanodeDump synchronizes it but other instances such as DatanodeManager#getDatanode() does not. I think you are talking about FSNamesystem.heartbeatCheck(). I incorrectly removed the synchronization. Fixed it. 4. In a future patch FSNamesystem#heartbeats and its synchronization should move to blockmanager That's correct. 5. Please change javadoc of getDatanode() as throws UnregisteredNodeException. Done. 6. I am sure you have already planned for removing FSNamesystem#blockInvalidateLimit in future patch. Removed. 7. How is DatanodeManager#getDatanodeCyclicIteration() synchronized? It cannot be synchronized using FSNamesystem.writeLock() as done currently? It should accquire the write lock of DatanodeManager when we have read-write lock in DatanodeManager in the future.
        Hide
        Suresh Srinivas added a comment -

        Comments:

        1. Is this correct? 10000 * heartbeatIntervalSeconds Why 10000? Should you be multiplying it by 1000 when getting it from conf and not 10000 *?
        2. blockManager.getDatanodeManager().getDatanode() - we could add a method blockManager.getDataNode() - I prefer this instead of exposing DatanodeManager to FSNamesystem.
        3. Earlier getDatanode() used to lock using datanodeMap, do we need that with your patch? How is it synchronized now? Also datanodeMap access may not be synchronized correctly - for example datanodeDump synchronizes it but other instances such as DatanodeManager#getDatanode() does not.
        4. In a future patch FSNamesystem#heartbeats and its synchronization should move to blockmanager
        5. Please change javadoc of getDatanode() as throws UnregisteredNodeException.
        6. I am sure you have already planned for removing FSNamesystem#blockInvalidateLimit in future patch.
        7. How is DatanodeManager#getDatanodeCyclicIteration() synchronized? It cannot be synchronized using FSNamesystem.writeLock() as done currently?
        Show
        Suresh Srinivas added a comment - Comments: Is this correct? 10000 * heartbeatIntervalSeconds Why 10000? Should you be multiplying it by 1000 when getting it from conf and not 10000 *? blockManager.getDatanodeManager().getDatanode() - we could add a method blockManager.getDataNode() - I prefer this instead of exposing DatanodeManager to FSNamesystem. Earlier getDatanode() used to lock using datanodeMap, do we need that with your patch? How is it synchronized now? Also datanodeMap access may not be synchronized correctly - for example datanodeDump synchronizes it but other instances such as DatanodeManager#getDatanode() does not. In a future patch FSNamesystem#heartbeats and its synchronization should move to blockmanager Please change javadoc of getDatanode() as throws UnregisteredNodeException. I am sure you have already planned for removing FSNamesystem#blockInvalidateLimit in future patch. How is DatanodeManager#getDatanodeCyclicIteration() synchronized? It cannot be synchronized using FSNamesystem.writeLock() as done currently?
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12487587/h2191_20110723b.patch
        against trunk revision 1150067.

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

        +1 tests included. The patch appears to include 18 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

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

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

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

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        +1 system test framework. The patch passed system test framework compile.

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/1005//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/1005//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1005//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12487587/h2191_20110723b.patch against trunk revision 1150067. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 18 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/1005//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/1005//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1005//console This message is automatically generated.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        h2191_20110723b.patch: fixed the javadoc warnings.

        Show
        Tsz Wo Nicholas Sze added a comment - h2191_20110723b.patch: fixed the javadoc warnings.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12487585/h2191_20110723.patch
        against trunk revision 1149771.

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

        +1 tests included. The patch appears to include 18 new or modified tests.

        -1 javadoc. The javadoc tool appears to have generated 2 warning messages.

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

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

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

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        +1 system test framework. The patch passed system test framework compile.

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/1002//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/1002//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1002//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12487585/h2191_20110723.patch against trunk revision 1149771. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 18 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 2 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/1002//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/1002//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1002//console This message is automatically generated.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12487585/h2191_20110723.patch
        against trunk revision 1149771.

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

        +1 tests included. The patch appears to include 18 new or modified tests.

        -1 javadoc. The javadoc tool appears to have generated 2 warning messages.

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

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

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

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        +1 system test framework. The patch passed system test framework compile.

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/1003//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/1003//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1003//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12487585/h2191_20110723.patch against trunk revision 1149771. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 18 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 2 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/1003//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/1003//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1003//console This message is automatically generated.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        h2191_20110723.patch: 1st patch

        Show
        Tsz Wo Nicholas Sze added a comment - h2191_20110723.patch: 1st patch

          People

          • Assignee:
            Tsz Wo Nicholas Sze
            Reporter:
            Tsz Wo Nicholas Sze
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development