Hadoop Common
  1. Hadoop Common
  2. HADOOP-2633

Revert change to fsck made as part of permissions implementation

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.16.0
    • Fix Version/s: 0.16.0
    • Component/s: None
    • Labels:
      None

      Description

      Earlier change has unacceptable performance behavior.

      1. 2633_20080116.patch
        5 kB
        Tsz Wo Nicholas Sze
      2. 2633_20080117.patch
        7 kB
        Tsz Wo Nicholas Sze
      3. 2633_20080117b.patch
        7 kB
        Tsz Wo Nicholas Sze
      4. 2633_20080117c.patch
        7 kB
        Tsz Wo Nicholas Sze
      5. 2633_20080118.patch
        9 kB
        Tsz Wo Nicholas Sze
      6. 2633_20080122.patch
        14 kB
        Tsz Wo Nicholas Sze
      7. 2633_20080123.patch
        11 kB
        Tsz Wo Nicholas Sze

        Activity

        Hide
        Hudson added a comment -
        Show
        Hudson added a comment - Integrated in Hadoop-trunk #378 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/378/ )
        Hide
        Konstantin Shvachko added a comment -

        I just committed this. Thank you Nicholas.

        Show
        Konstantin Shvachko added a comment - I just committed this. Thank you Nicholas.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        2633_20080123.patch: put fsck unit tests back to one class, updated with current trunk.

        Show
        Tsz Wo Nicholas Sze added a comment - 2633_20080123.patch: put fsck unit tests back to one class, updated with current trunk.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12373782/2633_20080122.patch
        against trunk revision r614413.

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

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

        javac +1. The applied patch does not generate any new compiler warnings.

        findbugs +1. The patch does not introduce any new Findbugs warnings.

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

        contrib tests -1. The patch failed contrib unit tests.

        Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1681/testReport/
        Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1681/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1681/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1681/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/12373782/2633_20080122.patch against trunk revision r614413. @author +1. The patch does not contain any @author tags. javadoc +1. The javadoc tool did not generate any warning messages. javac +1. The applied patch does not generate any new compiler warnings. findbugs +1. The patch does not introduce any new Findbugs warnings. core tests +1. The patch passed core unit tests. contrib tests -1. The patch failed contrib unit tests. Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1681/testReport/ Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1681/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1681/artifact/trunk/build/test/checkstyle-errors.html Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1681/console This message is automatically generated.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        2633_20080122.patch: rewrite TestFsck so that MiniDFSCluster is shutdown properly between each test.

        Show
        Tsz Wo Nicholas Sze added a comment - 2633_20080122.patch: rewrite TestFsck so that MiniDFSCluster is shutdown properly between each test.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12373551/2633_20080118.patch
        against trunk revision r613359.

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

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

        javac +1. The applied patch does not generate any new compiler warnings.

        findbugs +1. The patch does not introduce any new Findbugs warnings.

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

        contrib tests -1. The patch failed contrib unit tests.

        Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1654/testReport/
        Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1654/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1654/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1654/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/12373551/2633_20080118.patch against trunk revision r613359. @author +1. The patch does not contain any @author tags. javadoc +1. The javadoc tool did not generate any warning messages. javac +1. The applied patch does not generate any new compiler warnings. findbugs +1. The patch does not introduce any new Findbugs warnings. core tests -1. The patch failed core unit tests. contrib tests -1. The patch failed contrib unit tests. Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1654/testReport/ Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1654/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1654/artifact/trunk/build/test/checkstyle-errors.html Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1654/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/12373464/2633_20080117c.patch
        against trunk revision r613115.

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

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

        javac +1. The applied patch does not generate any new compiler warnings.

        findbugs +1. The patch does not introduce any new Findbugs warnings.

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

        contrib tests -1. The patch failed contrib unit tests.

        Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1639/testReport/
        Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1639/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1639/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1639/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/12373464/2633_20080117c.patch against trunk revision r613115. @author +1. The patch does not contain any @author tags. javadoc +1. The javadoc tool did not generate any warning messages. javac +1. The applied patch does not generate any new compiler warnings. findbugs +1. The patch does not introduce any new Findbugs warnings. core tests -1. The patch failed core unit tests. contrib tests -1. The patch failed contrib unit tests. Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1639/testReport/ Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1639/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1639/artifact/trunk/build/test/checkstyle-errors.html Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1639/console This message is automatically generated.
        Hide
        Robert Chansler added a comment -

        This change should be very low risk--fsck either works or it doesn't. There
        was a lot more risk in the new code!

        In this context, make sure you've taken a peek at

        https://issues.apache.org/jira/browse/HADOOP-2632

        Show
        Robert Chansler added a comment - This change should be very low risk--fsck either works or it doesn't. There was a lot more risk in the new code! In this context, make sure you've taken a peek at https://issues.apache.org/jira/browse/HADOOP-2632
        Hide
        Nigel Daley added a comment -

        Does a test need to be written for this? It's not clear how easy that would be.

        Show
        Nigel Daley added a comment - Does a test need to be written for this? It's not clear how easy that would be.
        Hide
        Konstantin Shvachko added a comment -

        +1

        Show
        Konstantin Shvachko added a comment - +1
        Hide
        Tsz Wo Nicholas Sze added a comment -

        2633_20080117c.patch: re-arranged some codes.

        Show
        Tsz Wo Nicholas Sze added a comment - 2633_20080117c.patch: re-arranged some codes.
        Hide
        Tsz Wo Nicholas Sze added a comment - - edited

        2633_20080117b.patch: updated with trunk

        Show
        Tsz Wo Nicholas Sze added a comment - - edited 2633_20080117b.patch: updated with trunk
        Hide
        Tsz Wo Nicholas Sze added a comment -

        2633_20080117.patch: incorporated all suggestions. Thanks, Konstantin.

        Show
        Tsz Wo Nicholas Sze added a comment - 2633_20080117.patch: incorporated all suggestions. Thanks, Konstantin.
        Hide
        Konstantin Shvachko added a comment -
        • nn.namesystem.now() should be FSNamesystem.now()
        • Methods should be separate by a blank line.
        • there is to many methods called getBlockLocationsInternal(). It took me at least 20 minutes to understand who is calling whom. Traditionally the general idea of methods and their *Internal counterparts is to distinguish between the api methods and their synchronized parts. Synchronized part of the implementation is usually called *Internal. It is also supposed to be private.

        I propose the following modifications here:

        • getBlockLocationsInternal(String src,long,long) should be renamed to getBlockLocations(String src,long,long) because you need to call ii in NamenodeFsck.
        • getBlockLocationsInternal(String clientMachine,String src,long,long) should be removed and the sorting part of it should be placed directly into getBlockLocations(String clientMachine,String src,long,long).
        • the private getBlockLocationInternal(INodeFile, ...) should be renamed to getBlockLocationsInternal(INodeFile, ...) with an 's' in the middle. This was probably my fault.

        As a result you will have only one private synchronized getBlockLocationsInternal() and two getBlockLocations().

        Show
        Konstantin Shvachko added a comment - nn.namesystem.now() should be FSNamesystem.now() Methods should be separate by a blank line. there is to many methods called getBlockLocationsInternal(). It took me at least 20 minutes to understand who is calling whom. Traditionally the general idea of methods and their *Internal counterparts is to distinguish between the api methods and their synchronized parts. Synchronized part of the implementation is usually called *Internal. It is also supposed to be private. I propose the following modifications here: getBlockLocationsInternal(String src,long,long) should be renamed to getBlockLocations(String src,long,long) because you need to call ii in NamenodeFsck. getBlockLocationsInternal(String clientMachine,String src,long,long) should be removed and the sorting part of it should be placed directly into getBlockLocations(String clientMachine,String src,long,long). the private getBlockLocationInternal(INodeFile, ...) should be renamed to getBlockLocationsInternal(INodeFile, ...) with an 's' in the middle. This was probably my fault. As a result you will have only one private synchronized getBlockLocationsInternal() and two getBlockLocations().
        Hide
        Tsz Wo Nicholas Sze added a comment -

        2633_20080116.patch: access NameNode by (local) method calls instead of RPC.

        Show
        Tsz Wo Nicholas Sze added a comment - 2633_20080116.patch: access NameNode by (local) method calls instead of RPC.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development