Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.5.0
    • Component/s: namenode
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      This jira proposes:

      1. Cleaning up dead code in FSDirectory.
      2. Simplify the control flows that IntelliJ flags as warnings.
      3. Move functions related to resolving paths into one place.
      1. HDFS-6328.004.patch
        9 kB
        Haohui Mai
      2. HDFS-6328.003.patch
        9 kB
        Haohui Mai
      3. HDFS-6328.002.patch
        11 kB
        Haohui Mai
      4. HDFS-6328.001.patch
        11 kB
        Haohui Mai
      5. HDFS-6328.000.patch
        12 kB
        Haohui Mai

        Issue Links

          Activity

          Hide
          Haohui Mai added a comment -

          I've committed the patch to trunk and branch-2. Thanks Jing Zhao for the review.

          Show
          Haohui Mai added a comment - I've committed the patch to trunk and branch-2. Thanks Jing Zhao for the review.
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Hdfs-trunk #1753 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1753/)
          HDFS-6328. Clean up dead code in FSDirectory. Contributed by Haohui Mai. (wheat9: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1593755)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #1753 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1753/ ) HDFS-6328 . Clean up dead code in FSDirectory. Contributed by Haohui Mai. (wheat9: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1593755 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Mapreduce-trunk #1779 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1779/)
          HDFS-6328. Clean up dead code in FSDirectory. Contributed by Haohui Mai. (wheat9: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1593755)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #1779 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1779/ ) HDFS-6328 . Clean up dead code in FSDirectory. Contributed by Haohui Mai. (wheat9: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1593755 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Yarn-trunk #561 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/561/)
          HDFS-6328. Clean up dead code in FSDirectory. Contributed by Haohui Mai. (wheat9: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1593755)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #561 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/561/ ) HDFS-6328 . Clean up dead code in FSDirectory. Contributed by Haohui Mai. (wheat9: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1593755 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-trunk-Commit #5603 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5603/)
          HDFS-6328. Clean up dead code in FSDirectory. Contributed by Haohui Mai. (wheat9: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1593755)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #5603 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5603/ ) HDFS-6328 . Clean up dead code in FSDirectory. Contributed by Haohui Mai. (wheat9: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1593755 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
          Hide
          Hadoop QA added a comment -

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

          +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 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-hdfs-project/hadoop-hdfs.

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6873//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6873//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/12644155/HDFS-6328.004.patch against trunk revision . +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 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6873//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6873//console This message is automatically generated.
          Hide
          Haohui Mai added a comment -

          Attaching v4 patch to address Jing's comments. Daryn Sharp, do you have any more comments? I'll commit it later today if there is no more comment.

          Show
          Haohui Mai added a comment - Attaching v4 patch to address Jing's comments. Daryn Sharp , do you have any more comments? I'll commit it later today if there is no more comment.
          Hide
          Jing Zhao added a comment -

          The latest patch looks good to me. Only one nit: the comment "// src[i - 1] is the last common ancestor." should be moved after the while loop. Other than this +1 (Maybe you do not need to run Jenkins again after moving the comment).

          Show
          Jing Zhao added a comment - The latest patch looks good to me. Only one nit: the comment "// src [i - 1] is the last common ancestor." should be moved after the while loop. Other than this +1 (Maybe you do not need to run Jenkins again after moving the comment).
          Hide
          Hadoop QA added a comment -

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

          +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 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-hdfs-project/hadoop-hdfs.

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6863//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6863//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/12644043/HDFS-6328.003.patch against trunk revision . +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 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6863//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6863//console This message is automatically generated.
          Hide
          Haohui Mai added a comment -

          From the jenkins console:

          {color:red}-1 overall{color}.  Here are the results of testing the latest attachment 
            http://issues.apache.org/jira/secure/attachment/12643597/HDFS-6328.002.patch
            against trunk revision .
          
              {color:green}+1 @author{color}.  The patch does not contain any @author tags.
          
              {color:red}-1 tests included{color}.  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.
          
              {color:green}+1 javac{color}.  The applied patch does not increase the total number of javac compiler warnings.
          
              {color:green}+1 javadoc{color}.  There were no new javadoc warning messages.
          
              {color:green}+1 eclipse:eclipse{color}.  The patch built with eclipse:eclipse.
          
              {color:green}+1 findbugs{color}.  The patch does not introduce any new Findbugs (version 1.3.9) warnings.
          
              {color:green}+1 release audit{color}.  The applied patch does not increase the total number of release audit warnings.
          
              {color:red}-1 core tests{color}.  The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:
          
                            org.apache.hadoop.hdfs.server.balancer.TestBalancerWithNodeGroup
          
              {color:green}+1 contrib tests{color}.  The patch passed contrib unit tests.
          
          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6838//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6838//console
          
          This message is automatically generated.
          
          Show
          Haohui Mai added a comment - From the jenkins console: {color:red}-1 overall{color}. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12643597/HDFS-6328.002.patch against trunk revision . {color:green}+1 @author{color}. The patch does not contain any @author tags. {color:red}-1 tests included{color}. 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. {color:green}+1 javac{color}. The applied patch does not increase the total number of javac compiler warnings. {color:green}+1 javadoc{color}. There were no new javadoc warning messages. {color:green}+1 eclipse:eclipse{color}. The patch built with eclipse:eclipse. {color:green}+1 findbugs{color}. The patch does not introduce any new Findbugs (version 1.3.9) warnings. {color:green}+1 release audit{color}. The applied patch does not increase the total number of release audit warnings. {color:red}-1 core tests{color}. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.server.balancer.TestBalancerWithNodeGroup {color:green}+1 contrib tests{color}. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6838//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6838//console This message is automatically generated.
          Hide
          Haohui Mai added a comment -

          The v2 patch addresses Jing's comments.

          Show
          Haohui Mai added a comment - The v2 patch addresses Jing's comments.
          Hide
          Jing Zhao added a comment -

          Is there a reason why this loop needed to become more complicated? At this point I believe it's guaranteed that the src & dest are not identical, nor is the src a subdir of the dest?

          Good catch, Daryn Sharp. I should catch this in my first review.

          As a general statement, I'm not sure there's a lot of value add in the changes like altering whitespace and moving methods.

          Although some changes like changing for loop to while loop is not necessary, I think this is still good cleanup, especially for those changes in the two unprotectedRename methods. One more thing we should do is to put common code of the two unprotectedRename methods into separate methods. But I'm fine with doing this in a separate jira.

          For the latest patch, the following change is unnecessary:

          -        src.endsWith(HdfsConstants.SEPARATOR_DOT_SNAPSHOT_DIR), 
          -        "%s does not end with %s", src, HdfsConstants.SEPARATOR_DOT_SNAPSHOT_DIR);
          +            src.endsWith(HdfsConstants.SEPARATOR_DOT_SNAPSHOT_DIR),
          +            "%s does not end with %s", src, HdfsConstants.SEPARATOR_DOT_SNAPSHOT_DIR);
          

          Also, maybe we do not need to move the set of getINode methods. As Daryn Sharp pointed out, these changes can make merging harder (e.g., merging changes from 2.x to 0.23).

          Show
          Jing Zhao added a comment - Is there a reason why this loop needed to become more complicated? At this point I believe it's guaranteed that the src & dest are not identical, nor is the src a subdir of the dest? Good catch, Daryn Sharp . I should catch this in my first review. As a general statement, I'm not sure there's a lot of value add in the changes like altering whitespace and moving methods. Although some changes like changing for loop to while loop is not necessary, I think this is still good cleanup, especially for those changes in the two unprotectedRename methods. One more thing we should do is to put common code of the two unprotectedRename methods into separate methods. But I'm fine with doing this in a separate jira. For the latest patch, the following change is unnecessary: - src.endsWith(HdfsConstants.SEPARATOR_DOT_SNAPSHOT_DIR), - "%s does not end with %s" , src, HdfsConstants.SEPARATOR_DOT_SNAPSHOT_DIR); + src.endsWith(HdfsConstants.SEPARATOR_DOT_SNAPSHOT_DIR), + "%s does not end with %s" , src, HdfsConstants.SEPARATOR_DOT_SNAPSHOT_DIR); Also, maybe we do not need to move the set of getINode methods. As Daryn Sharp pointed out, these changes can make merging harder (e.g., merging changes from 2.x to 0.23).
          Hide
          Haohui Mai added a comment -

          Thanks Daryn Sharp for looking at this. The v2 patch addresses your comments about the loop. Does it look okay to you? I'd like to move forward to get both HDFS-6330 and HDFS-6315 going.

          Show
          Haohui Mai added a comment - Thanks Daryn Sharp for looking at this. The v2 patch addresses your comments about the loop. Does it look okay to you? I'd like to move forward to get both HDFS-6330 and HDFS-6315 going.
          Hide
          Hadoop QA added a comment -

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

          +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 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.server.balancer.TestBalancerWithNodeGroup

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6838//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6838//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/12643597/HDFS-6328.002.patch against trunk revision . +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 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.server.balancer.TestBalancerWithNodeGroup +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6838//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6838//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/12643430/HDFS-6328.001.patch
          against trunk revision .

          +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 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-hdfs-project/hadoop-hdfs.

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6829//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6829//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/12643430/HDFS-6328.001.patch against trunk revision . +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 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6829//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6829//console This message is automatically generated.
          Hide
          Haohui Mai added a comment -

          The main motivation of this jira is to perform identical code clean up to make the reviews of HDFS-6330 and HDFS-6315 easier.

          The original motivation is to make sure there is no OutOfBoundException in the loop locally without going through all the traces, but it looks the order of the clauses is wrong. I'll fix it in another patch.

          Show
          Haohui Mai added a comment - The main motivation of this jira is to perform identical code clean up to make the reviews of HDFS-6330 and HDFS-6315 easier. The original motivation is to make sure there is no OutOfBoundException in the loop locally without going through all the traces, but it looks the order of the clauses is wrong. I'll fix it in another patch.
          Hide
          Daryn Sharp added a comment -

          As a general statement, I'm not sure there's a lot of value add in the changes like altering whitespace and moving methods. Mixing functional changes and cosmetic changes make it a bit harder to see what actually changed. Please understand it does makes life harder for those of us also working in the code that will encounter merge conflicts...

          Is there a reason why this loop needed to become more complicated? At this point I believe it's guaranteed that the src & dest are not identical, nor is the src a subdir of the dest?

          -    for(; src[i] == dst[i]; i++);
               // src[i - 1] is the last common ancestor.
          +    while(src[i] == dst[i] && i < src.length && i < dst.length) {
          +      i++;
          +    }
          
          Show
          Daryn Sharp added a comment - As a general statement, I'm not sure there's a lot of value add in the changes like altering whitespace and moving methods. Mixing functional changes and cosmetic changes make it a bit harder to see what actually changed. Please understand it does makes life harder for those of us also working in the code that will encounter merge conflicts... Is there a reason why this loop needed to become more complicated? At this point I believe it's guaranteed that the src & dest are not identical, nor is the src a subdir of the dest? - for (; src[i] == dst[i]; i++); // src[i - 1] is the last common ancestor. + while (src[i] == dst[i] && i < src.length && i < dst.length) { + i++; + }
          Hide
          Jing Zhao added a comment -

          The patch looks pretty good to me. Thanks for the cleanup. Some minors:

          1. The changes on imports seem unnecessary
          2. The following change may need to be reverted:
            -    Preconditions.checkArgument(
            -        src.endsWith(HdfsConstants.SEPARATOR_DOT_SNAPSHOT_DIR), 
            -        "%s does not end with %s", src, HdfsConstants.SEPARATOR_DOT_SNAPSHOT_DIR);
            +    Preconditions.checkArgument(src.endsWith(HdfsConstants.SEPARATOR_DOT_SNAPSHOT_DIR), "%s does not end with %s", src, HdfsConstants.SEPARATOR_DOT_SNAPSHOT_DIR);
            
          3. The following line exceeds 80 characters:
            +      return srcs.startsWith("/") && !srcs.endsWith("/") && getINode4Write(srcs, false) == null;
            
          4. Let's add "{}" for the while loop:
            +    while(src[i] == dst[i])
            +      i++;
            

          +1 after addressing the comments.

          Show
          Jing Zhao added a comment - The patch looks pretty good to me. Thanks for the cleanup. Some minors: The changes on imports seem unnecessary The following change may need to be reverted: - Preconditions.checkArgument( - src.endsWith(HdfsConstants.SEPARATOR_DOT_SNAPSHOT_DIR), - "%s does not end with %s" , src, HdfsConstants.SEPARATOR_DOT_SNAPSHOT_DIR); + Preconditions.checkArgument(src.endsWith(HdfsConstants.SEPARATOR_DOT_SNAPSHOT_DIR), "%s does not end with %s" , src, HdfsConstants.SEPARATOR_DOT_SNAPSHOT_DIR); The following line exceeds 80 characters: + return srcs.startsWith( "/" ) && !srcs.endsWith( "/" ) && getINode4Write(srcs, false ) == null ; Let's add "{}" for the while loop: + while (src[i] == dst[i]) + i++; +1 after addressing the comments.
          Hide
          Haohui Mai added a comment -

          The test failure is unrelated. I ran the test locally and it passed.

          Show
          Haohui Mai added a comment - The test failure is unrelated. I ran the test locally and it passed.
          Hide
          Hadoop QA added a comment -

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

          +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 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.server.datanode.TestBPOfferService

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6819//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6819//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/12643135/HDFS-6328.000.patch against trunk revision . +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 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.server.datanode.TestBPOfferService +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6819//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6819//console This message is automatically generated.

            People

            • Assignee:
              Haohui Mai
              Reporter:
              Haohui Mai
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development