Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-880

TestNNLeaseRecovery fails on windows

    Details

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

      Description

      TestNNLeaseRecovery fails on windows trying to delete name-node storage directory.

      1. testNNLeaseRecovery.patch
        3 kB
        Konstantin Shvachko
      2. HDFS-880.patch
        3 kB
        Konstantin Boudnik

        Activity

        Hide
        Konstantin Shvachko added a comment -

        The attached patch is not a fix for the problem. But it helps to see where the problem is.

        • In the patch I set explicitly name-node storage directories. In the original code it is supposed that directories are in /tmp/hadoop-shv/dfs, but if you run it in eclipse it may not be (and usually is not) the case.
        • There is no need to separately close the FSImage and then FSNamesystem. FSNamesystem.close() will also close FSImage.
        • Use FSUtil.fullyDelete() it is standard.
        Show
        Konstantin Shvachko added a comment - The attached patch is not a fix for the problem. But it helps to see where the problem is. In the patch I set explicitly name-node storage directories. In the original code it is supposed that directories are in /tmp/hadoop-shv/dfs, but if you run it in eclipse it may not be (and usually is not) the case. There is no need to separately close the FSImage and then FSNamesystem. FSNamesystem.close() will also close FSImage. Use FSUtil.fullyDelete() it is standard.
        Hide
        Konstantin Shvachko added a comment -

        Looks like the problem is in TestNNLeaseRecovery.mockFileBlocks(). FSNamesystem constructor creates FSImage, which opens image and edits files. Then mockFileBlocks() creates a mocked FSDirectory, where FSImage is null, and assigns it to FSNamesystem.dir. Now when FSNamesystem closes it will keep the original files open since it does not have access to them (the original FSImage was replaced by null).
        This does not effect the test in Unix, because it lets remove opened files. NTFS does not allow it.

        Show
        Konstantin Shvachko added a comment - Looks like the problem is in TestNNLeaseRecovery.mockFileBlocks() . FSNamesystem constructor creates FSImage, which opens image and edits files. Then mockFileBlocks() creates a mocked FSDirectory , where FSImage is null, and assigns it to FSNamesystem.dir . Now when FSNamesystem closes it will keep the original files open since it does not have access to them (the original FSImage was replaced by null). This does not effect the test in Unix, because it lets remove opened files. NTFS does not allow it.
        Hide
        Konstantin Boudnik added a comment -

        The updated version of Konstantin's patch does make sure that there's no unclosed instance of FSDirectory is laying around: I'm simply closing it before the change of the reference.

        The reason why it has to be a mocked reference to FSDirectory is that mocked INodeFileUnderConstruction has to be returned for the purpose having properly set blocks and so on.

        In fact, with having real FSDirectory's instance being properly closed the whole business of removing the NAME_DIR isn't needed any more, but I'll leave for a sanity sake.

        Show
        Konstantin Boudnik added a comment - The updated version of Konstantin's patch does make sure that there's no unclosed instance of FSDirectory is laying around: I'm simply closing it before the change of the reference. The reason why it has to be a mocked reference to FSDirectory is that mocked INodeFileUnderConstruction has to be returned for the purpose having properly set blocks and so on. In fact, with having real FSDirectory's instance being properly closed the whole business of removing the NAME_DIR isn't needed any more, but I'll leave for a sanity sake.
        Hide
        Konstantin Shvachko added a comment -

        I ran the test. It worked on my windows box.
        Is it possible to instantiate FSNamesystem using the constructor that takes FSImage as a parameter. Just a thought to verify, if it is not easy please submit the patch as it is. I remember Eli was working on mocking FSNamesystem, may he knows how to it better.

        Show
        Konstantin Shvachko added a comment - I ran the test. It worked on my windows box. Is it possible to instantiate FSNamesystem using the constructor that takes FSImage as a parameter. Just a thought to verify, if it is not easy please submit the patch as it is. I remember Eli was working on mocking FSNamesystem, may he knows how to it better.
        Hide
        Konstantin Boudnik added a comment -

        Is it possible to instantiate FSNamesystem using the constructor that takes FSImage as a parameter.

        I think it makes little difference because no matter what constructor is used FSDirectory member will be initialized (currently through initialize(Configuration conf, FSImage fsImage) call where fsImage equals null; or through direct {{ this.dir = new FSDirectory(fsImage, this, conf);}} in your case. It still leave us with two FSDirectory objects and so on.

        I'll gonna commit the latest (combined) version of the patch. Thanks for finding and debugging this issue: hard to find problem indeed!

        Show
        Konstantin Boudnik added a comment - Is it possible to instantiate FSNamesystem using the constructor that takes FSImage as a parameter. I think it makes little difference because no matter what constructor is used FSDirectory member will be initialized (currently through initialize(Configuration conf, FSImage fsImage) call where fsImage equals null ; or through direct {{ this.dir = new FSDirectory(fsImage, this, conf);}} in your case. It still leave us with two FSDirectory objects and so on. I'll gonna commit the latest (combined) version of the patch. Thanks for finding and debugging this issue: hard to find problem indeed!
        Hide
        Konstantin Boudnik added a comment -

        I've committed it to the trunk and 0.21 branch.

        Show
        Konstantin Boudnik added a comment - I've committed it to the trunk and 0.21 branch.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk-Commit #165 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/165/)
        . TestNNLeaseRecovery fails on windows. Contributed by Konstantin Boudnik, Konstantin Shvachko.

        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #165 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/165/ ) . TestNNLeaseRecovery fails on windows. Contributed by Konstantin Boudnik, Konstantin Shvachko.
        Hide
        Eli Collins added a comment -

        Is it possible to instantiate FSNamesystem using the constructor that takes FSImage as a parameter. Just a thought to verify, if it is not easy please submit the patch as it is. I remember Eli was working on mocking FSNamesystem, may he knows how to it better.

        My first patch for HDFS-669 mocked up FSNamesystem, it's doable. Per our discussion a while back we decided to some unit tests directly against NameNode. Working on finishing up the symlink patches first.

        Show
        Eli Collins added a comment - Is it possible to instantiate FSNamesystem using the constructor that takes FSImage as a parameter. Just a thought to verify, if it is not easy please submit the patch as it is. I remember Eli was working on mocking FSNamesystem, may he knows how to it better. My first patch for HDFS-669 mocked up FSNamesystem, it's doable. Per our discussion a while back we decided to some unit tests directly against NameNode. Working on finishing up the symlink patches first.
        Hide
        Konstantin Boudnik added a comment -

        Yes, pure mocking of FSNamesystem is doable, but clearly for this testing's scope it seems to be much easier to spy on it and replace some references with mocks.

        Show
        Konstantin Boudnik added a comment - Yes, pure mocking of FSNamesystem is doable, but clearly for this testing's scope it seems to be much easier to spy on it and replace some references with mocks.
        Hide
        Hudson added a comment -

        Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #94 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/94/)
        . TestNNLeaseRecovery fails on windows. Contributed by Konstantin Boudnik, Konstantin Shvachko.

        Show
        Hudson added a comment - Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #94 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/94/ ) . TestNNLeaseRecovery fails on windows. Contributed by Konstantin Boudnik, Konstantin Shvachko.
        Hide
        Hudson added a comment -

        Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #183 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/183/)

        Show
        Hudson added a comment - Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #183 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/183/ )
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #198 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/198/)
        . TestNNLeaseRecovery fails on windows. Contributed by Konstantin Boudnik, Konstantin Shvachko.

        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #198 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/198/ ) . TestNNLeaseRecovery fails on windows. Contributed by Konstantin Boudnik, Konstantin Shvachko.
        Hide
        Eli Collins added a comment -

        Yes, pure mocking of FSNamesystem is doable, but clearly for this testing's scope it seems to be much easier to spy on it and replace some references with mocks.

        Sorry, wasn't clear, the patch for FSNamesystem actually spies on FSNamesystem:

        // Returns a spied on FSNamesystem so tests can hook its methods
        FSNamesystem getNamesystem() throws IOException {
          Configuration conf = new HdfsConfiguration();
           // avoid stubbing access control
          conf.setBoolean("dfs.permissions.enabled", false); 
          NameNode.initMetrics(conf, NamenodeRole.ACTIVE);
          return spy(new FSNamesystem(conf));
        }
        
        Show
        Eli Collins added a comment - Yes, pure mocking of FSNamesystem is doable, but clearly for this testing's scope it seems to be much easier to spy on it and replace some references with mocks. Sorry, wasn't clear, the patch for FSNamesystem actually spies on FSNamesystem: // Returns a spied on FSNamesystem so tests can hook its methods FSNamesystem getNamesystem() throws IOException { Configuration conf = new HdfsConfiguration(); // avoid stubbing access control conf.setBoolean( "dfs.permissions.enabled" , false ); NameNode.initMetrics(conf, NamenodeRole.ACTIVE); return spy( new FSNamesystem(conf)); }

          People

          • Assignee:
            Konstantin Boudnik
            Reporter:
            Konstantin Shvachko
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development