Details

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

      Description

      Inner class FSDir constructor is doing duplicate iterations over the listed files in the passed directory. We can optimize this to single loop and also we can avoid isDirectory check which will perform some native invocations.

      Consider a case: one directory has only one child directory and 10000 files.

      1) First loop will get the number of children directories.

      2) if (numChildren > 0) , This condition will satisfy and again it will iterate 10001 times and also will check isDirectory.

      1. HDFS-1774-1.patch
        1 kB
        Uma Maheswara Rao G
      2. HDFS-1774-1.patch
        1 kB
        Uma Maheswara Rao G
      3. HDFS-1774.patch
        5 kB
        Uma Maheswara Rao G

        Activity

        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk-Commit #796 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/796/)
        HDFS-1774. Small optimization to FSDataset. Contributed by Uma Maheswara Rao G

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

        • /hadoop/common/trunk/hdfs/CHANGES.txt
        • /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/datanode/FSDataset.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #796 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/796/ ) HDFS-1774 . Small optimization to FSDataset. Contributed by Uma Maheswara Rao G eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1148894 Files : /hadoop/common/trunk/hdfs/CHANGES.txt /hadoop/common/trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/datanode/FSDataset.java
        Hide
        Uma Maheswara Rao G added a comment -

        Thanks a lot for Nicholas and Eli for Review

        Show
        Uma Maheswara Rao G added a comment - Thanks a lot for Nicholas and Eli for Review
        Hide
        Eli Collins added a comment -

        I've committed this. Thanks Uma!

        Show
        Eli Collins added a comment - I've committed this. Thanks Uma!
        Hide
        Eli Collins added a comment -

        Sorry, I forgot that children was an array, your makes sense.

        I'm +1 on the latest patch as well.

        Show
        Eli Collins added a comment - Sorry, I forgot that children was an array, your makes sense. I'm +1 on the latest patch as well.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        +1 change look good.

        Eli, do you have further comments?

        Show
        Tsz Wo Nicholas Sze added a comment - +1 change look good. Eli, do you have further comments?
        Hide
        Uma Maheswara Rao G added a comment -

        This change is just kind of re-factoring. No tests required.

        Show
        Uma Maheswara Rao G added a comment - This change is just kind of re-factoring. No tests required.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12486886/HDFS-1774-1.patch
        against trunk revision 1147980.

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +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/962//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/962//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/962//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/12486886/HDFS-1774-1.patch against trunk revision 1147980. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +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/962//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/962//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/962//console This message is automatically generated.
        Hide
        Uma Maheswara Rao G added a comment -

        Hi Eli,
        I am little bit confusing with the question

        You are talking about this line?

          if (dirList.size() > 0) {
        +          children = dirList.toArray(new FSDir[dirList.size()]);
                 }
        

        if i am not wrong: this is because i can not assign directly list to array right?

        Since i can not guess the children size, i have chosen list initially to collect the elements.

        please correct me if i understood wrongly

        Show
        Uma Maheswara Rao G added a comment - Hi Eli, I am little bit confusing with the question You are talking about this line? if (dirList.size() > 0) { + children = dirList.toArray( new FSDir[dirList.size()]); } if i am not wrong: this is because i can not assign directly list to array right? Since i can not guess the children size, i have chosen list initially to collect the elements. please correct me if i understood wrongly
        Hide
        Eli Collins added a comment -

        I understand the fix, I'm asking why you need to copy the array, seems like assigning it to child is sufficient no?

        Show
        Eli Collins added a comment - I understand the fix, I'm asking why you need to copy the array, seems like assigning it to child is sufficient no?
        Hide
        Uma Maheswara Rao G added a comment -

        Thanks a lot Eli, for taking a look on this patch.

        Here since children is an array, i am just adding the elements into list and converting it to array if it has elements.

        old code will just loop two times and perform isDirectory checks twice.

        Show
        Uma Maheswara Rao G added a comment - Thanks a lot Eli, for taking a look on this patch. Here since children is an array, i am just adding the elements into list and converting it to array if it has elements. old code will just loop two times and perform isDirectory checks twice.
        Hide
        Eli Collins added a comment -

        Why not just assign dirList to children (vs copying it)? dirList isn't used anywhere, you add it in this patch.

        Show
        Eli Collins added a comment - Why not just assign dirList to children (vs copying it)? dirList isn't used anywhere, you add it in this patch.
        Hide
        Uma Maheswara Rao G added a comment -

        Re submitted the same patch to trigger hudson.

        Show
        Uma Maheswara Rao G added a comment - Re submitted the same patch to trigger hudson.
        Hide
        Uma Maheswara Rao G added a comment -

        Attached the Patch for Review.

        Show
        Uma Maheswara Rao G added a comment - Attached the Patch for Review.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12474509/HDFS-1774.patch
        against trunk revision 1083958.

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

        +1 tests included. The patch appears to include 3 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:
        org.apache.hadoop.cli.TestHDFSCLI
        org.apache.hadoop.hdfs.server.datanode.TestBlockReport
        org.apache.hadoop.hdfs.server.datanode.TestTransferRbw
        org.apache.hadoop.hdfs.TestDFSShell

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

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

        Test results: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/282//testReport/
        Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/282//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/282//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/12474509/HDFS-1774.patch against trunk revision 1083958. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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: org.apache.hadoop.cli.TestHDFSCLI org.apache.hadoop.hdfs.server.datanode.TestBlockReport org.apache.hadoop.hdfs.server.datanode.TestTransferRbw org.apache.hadoop.hdfs.TestDFSShell -1 contrib tests. The patch failed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/282//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/282//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/282//console This message is automatically generated.

          People

          • Assignee:
            Uma Maheswara Rao G
            Reporter:
            Uma Maheswara Rao G
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development