Uploaded image for project: 'Hadoop HDFS'
  1. Hadoop HDFS
  2. HDFS-10797

Disk usage summary of snapshots causes renamed blocks to get counted twice

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.8.0
    • Fix Version/s: 2.8.0, 3.0.0-alpha2
    • Component/s: snapshots
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Hide
      Disk usage summaries previously incorrectly counted files twice if they had been renamed (including files moved to Trash) since being snapshotted. Summaries now include current data plus snapshotted data that is no longer under the directory either due to deletion or being moved outside of the directory.
      Show
      Disk usage summaries previously incorrectly counted files twice if they had been renamed (including files moved to Trash) since being snapshotted. Summaries now include current data plus snapshotted data that is no longer under the directory either due to deletion or being moved outside of the directory.

      Description

      DirectoryWithSnapshotFeature.computeContentSummary4Snapshot calculates how much disk usage is used by a snapshot by tallying up the files in the snapshot that have since been deleted (that way it won't overlap with regular files whose disk usage is computed separately). However that is determined from a diff that shows moved (to Trash or otherwise) or renamed files as a deletion and a creation operation that may overlap with the list of blocks. Only the deletion operation is taken into consideration, and this causes those blocks to get represented twice in the disk usage tallying.

      1. HDFS-10797.001.patch
        6 kB
        Sean Mackrory
      2. HDFS-10797.002.patch
        7 kB
        Sean Mackrory
      3. HDFS-10797.003.patch
        7 kB
        Sean Mackrory
      4. HDFS-10797.004.patch
        11 kB
        Sean Mackrory
      5. HDFS-10797.005.patch
        21 kB
        Sean Mackrory
      6. HDFS-10797.006.patch
        25 kB
        Sean Mackrory
      7. HDFS-10797.007.patch
        24 kB
        Sean Mackrory
      8. HDFS-10797.008.patch
        24 kB
        Sean Mackrory
      9. HDFS-10797.009.patch
        25 kB
        Sean Mackrory
      10. HDFS-10797.010.patch
        25 kB
        Sean Mackrory
      11. HDFS-10797.010.patch
        25 kB
        Sean Mackrory

        Issue Links

          Activity

          Hide
          mackrorysd Sean Mackrory added a comment -

          To reproduce the discrepancy you can follow the following procedure. I put a 100 MB file into HDFS and snapshot it (hadoop fs -du -s reports 100 MB * replication after both operations), and then append another 100 MB onto it (hadoop fs -du -s will report 200 MB * replication factor at that point). If I move the file to trash or simply rename it, hadoop fs -du -s starts reporting 300 MB * replication factor in the second column. I believe at this point it is counting some of the overlap in block between the snapshot and the regular file twice, because it views the move operation the same as a delete, but since the file wasn't actually deleted it gets counted again.

          dd if=/dev/zero of=100MB.zero bs=10000 count=10000
          bin/hadoop fs -mkdir -p /user/sean
          bin/hadoop fs -chown sean /user/sean
          bin/hadoop fs -put 100MB.zero /user/sean/HDFS-10797

          bin/hdfs dfsadmin -allowSnapshot /user/sean
          bin/hdfs dfs -createSnapshot /user/sean s1

          bin/hadoop fs -appendToFile 100MB.zero /user/sean/HDFS-10797

          bin/hadoop fs -du -s /user/sean

          bin/hadoop fs -rm /user/sean/HDFS-10797 # or simply rename with mv
          bin/hadoop fs -du -s /user/sean

          Show
          mackrorysd Sean Mackrory added a comment - To reproduce the discrepancy you can follow the following procedure. I put a 100 MB file into HDFS and snapshot it (hadoop fs -du -s reports 100 MB * replication after both operations), and then append another 100 MB onto it (hadoop fs -du -s will report 200 MB * replication factor at that point). If I move the file to trash or simply rename it, hadoop fs -du -s starts reporting 300 MB * replication factor in the second column. I believe at this point it is counting some of the overlap in block between the snapshot and the regular file twice, because it views the move operation the same as a delete, but since the file wasn't actually deleted it gets counted again. dd if=/dev/zero of=100MB.zero bs=10000 count=10000 bin/hadoop fs -mkdir -p /user/sean bin/hadoop fs -chown sean /user/sean bin/hadoop fs -put 100MB.zero /user/sean/ HDFS-10797 bin/hdfs dfsadmin -allowSnapshot /user/sean bin/hdfs dfs -createSnapshot /user/sean s1 bin/hadoop fs -appendToFile 100MB.zero /user/sean/ HDFS-10797 bin/hadoop fs -du -s /user/sean bin/hadoop fs -rm /user/sean/ HDFS-10797 # or simply rename with mv bin/hadoop fs -du -s /user/sean
          Hide
          mackrorysd Sean Mackrory added a comment -

          It's worth pointing out that the df command also shows some potentially surprising increases during this procedure, but I believe the behavior is correct. It increases more than you'd expect when appending data, but I believe this is because the snapshotted block and the amended block now need to be different blocks. The increase doesn't happen when using block sizes that divide 100MB evenly, which supports this idea.

          Show
          mackrorysd Sean Mackrory added a comment - It's worth pointing out that the df command also shows some potentially surprising increases during this procedure, but I believe the behavior is correct. It increases more than you'd expect when appending data, but I believe this is because the snapshotted block and the amended block now need to be different blocks. The increase doesn't happen when using block sizes that divide 100MB evenly, which supports this idea.
          Hide
          mackrorysd Sean Mackrory added a comment -

          Just a note for anyone reading my comments above, I learned that snapshots don't really work by maintaining references to immutable blocks, they work by maintaining a diff from the current state. So it's not a matter of counting up the correct blocks, it's a matter of ensuring renames, appends, etc. or properly accounted for from the diff. That can get tricky, since a file could get renamed multiple times, etc.

          Show
          mackrorysd Sean Mackrory added a comment - Just a note for anyone reading my comments above, I learned that snapshots don't really work by maintaining references to immutable blocks, they work by maintaining a diff from the current state. So it's not a matter of counting up the correct blocks, it's a matter of ensuring renames, appends, etc. or properly accounted for from the diff. That can get tricky, since a file could get renamed multiple times, etc.
          Hide
          mackrorysd Sean Mackrory added a comment -

          Attaching a patch that tries to identify files where the underlying inodes appear in both the DELETED and CREATED portion of the snapshot's diff, and does not count them toward the snapshot's space like it would a simply deleted file. Also added a test case that runs through scenarios like a chain of multiple renames, renaming a file and replacing the original file, and appends (even though they turned out to not have anything to do with the actual bug).

          Show
          mackrorysd Sean Mackrory added a comment - Attaching a patch that tries to identify files where the underlying inodes appear in both the DELETED and CREATED portion of the snapshot's diff, and does not count them toward the snapshot's space like it would a simply deleted file. Also added a test case that runs through scenarios like a chain of multiple renames, renaming a file and replacing the original file, and appends (even though they turned out to not have anything to do with the actual bug).
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 7m 21s trunk passed
          +1 compile 0m 44s trunk passed
          +1 checkstyle 0m 28s trunk passed
          +1 mvnsite 0m 51s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 39s trunk passed
          +1 javadoc 0m 54s trunk passed
          +1 mvninstall 0m 45s the patch passed
          +1 compile 0m 41s the patch passed
          +1 javac 0m 41s the patch passed
          -0 checkstyle 0m 24s hadoop-hdfs-project/hadoop-hdfs: The patch generated 1 new + 143 unchanged - 0 fixed = 144 total (was 143)
          +1 mvnsite 0m 48s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 45s the patch passed
          +1 javadoc 0m 50s the patch passed
          -1 unit 58m 53s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          78m 16s



          Reason Tests
          Failed junit tests hadoop.hdfs.TestDFSShell
            hadoop.hdfs.TestDecommissionWithStriped



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HDFS-10797
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828499/HDFS-10797.001.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 15e9d1d42a17 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 40b5a59
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/16752/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/16752/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16752/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16752/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 17s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 7m 21s trunk passed +1 compile 0m 44s trunk passed +1 checkstyle 0m 28s trunk passed +1 mvnsite 0m 51s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 39s trunk passed +1 javadoc 0m 54s trunk passed +1 mvninstall 0m 45s the patch passed +1 compile 0m 41s the patch passed +1 javac 0m 41s the patch passed -0 checkstyle 0m 24s hadoop-hdfs-project/hadoop-hdfs: The patch generated 1 new + 143 unchanged - 0 fixed = 144 total (was 143) +1 mvnsite 0m 48s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 45s the patch passed +1 javadoc 0m 50s the patch passed -1 unit 58m 53s hadoop-hdfs in the patch failed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 78m 16s Reason Tests Failed junit tests hadoop.hdfs.TestDFSShell   hadoop.hdfs.TestDecommissionWithStriped Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-10797 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828499/HDFS-10797.001.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 15e9d1d42a17 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 40b5a59 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/16752/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/16752/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16752/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16752/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          mackrorysd Sean Mackrory added a comment -

          Adding a doc comment to the test and fixing the checkstyle issue. I don't believe the 2 test failures are related to my change.

          Show
          mackrorysd Sean Mackrory added a comment - Adding a doc comment to the test and fixing the checkstyle issue. I don't believe the 2 test failures are related to my change.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 18s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 7m 32s trunk passed
          +1 compile 0m 57s trunk passed
          +1 checkstyle 0m 28s trunk passed
          +1 mvnsite 0m 54s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 1m 48s trunk passed
          +1 javadoc 0m 58s trunk passed
          +1 mvninstall 0m 53s the patch passed
          +1 compile 0m 46s the patch passed
          +1 javac 0m 46s the patch passed
          +1 checkstyle 0m 25s the patch passed
          +1 mvnsite 0m 50s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 2m 0s the patch passed
          +1 javadoc 0m 53s the patch passed
          +1 unit 56m 31s hadoop-hdfs in the patch passed.
          +1 asflicense 0m 18s The patch does not generate ASF License warnings.
          77m 7s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HDFS-10797
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828709/HDFS-10797.002.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 96ce17cd020e 3.13.0-92-generic #139-Ubuntu SMP Tue Jun 28 20:42:26 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / fcbac00
          Default Java 1.8.0_101
          findbugs v3.0.0
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16754/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16754/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 18s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 7m 32s trunk passed +1 compile 0m 57s trunk passed +1 checkstyle 0m 28s trunk passed +1 mvnsite 0m 54s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 1m 48s trunk passed +1 javadoc 0m 58s trunk passed +1 mvninstall 0m 53s the patch passed +1 compile 0m 46s the patch passed +1 javac 0m 46s the patch passed +1 checkstyle 0m 25s the patch passed +1 mvnsite 0m 50s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 2m 0s the patch passed +1 javadoc 0m 53s the patch passed +1 unit 56m 31s hadoop-hdfs in the patch passed. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 77m 7s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-10797 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828709/HDFS-10797.002.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 96ce17cd020e 3.13.0-92-generic #139-Ubuntu SMP Tue Jun 28 20:42:26 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / fcbac00 Default Java 1.8.0_101 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16754/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16754/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Sean Mackrory for reporting and working on this!
          I think you analysis is right, but would love to have Jing Zhao's input as well.

          The patch looks great, I only have 2 small comments:

          • We can add a test timeout
          • Do you think it's worth adding a final scenario to the new test, that after everything is deleted (including snapshots) the size goes back to 0?
          Show
          xiaochen Xiao Chen added a comment - Thanks Sean Mackrory for reporting and working on this! I think you analysis is right, but would love to have Jing Zhao 's input as well. The patch looks great, I only have 2 small comments: We can add a test timeout Do you think it's worth adding a final scenario to the new test, that after everything is deleted (including snapshots) the size goes back to 0?
          Hide
          mackrorysd Sean Mackrory added a comment -

          Thanks for the review, Xiao Chen. I do think that test is worth adding. Attaching a patch with that (it passes) and a timeout.

          Show
          mackrorysd Sean Mackrory added a comment - Thanks for the review, Xiao Chen . I do think that test is worth adding. Attaching a patch with that (it passes) and a timeout.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 22s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 7m 56s trunk passed
          +1 compile 0m 49s trunk passed
          +1 checkstyle 0m 30s trunk passed
          +1 mvnsite 0m 57s trunk passed
          +1 mvneclipse 0m 15s trunk passed
          +1 findbugs 1m 47s trunk passed
          +1 javadoc 0m 57s trunk passed
          +1 mvninstall 0m 50s the patch passed
          +1 compile 0m 44s the patch passed
          +1 javac 0m 44s the patch passed
          +1 checkstyle 0m 25s the patch passed
          +1 mvnsite 0m 48s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 47s the patch passed
          +1 javadoc 0m 55s the patch passed
          -1 unit 88m 23s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 30s The patch does not generate ASF License warnings.
          109m 34s



          Reason Tests
          Failed junit tests hadoop.hdfs.server.datanode.TestDataNodeUUID
            hadoop.hdfs.TestCrcCorruption
            hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure
          Timed out junit tests org.apache.hadoop.hdfs.TestLeaseRecovery2



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HDFS-10797
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828842/HDFS-10797.003.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 31121d442c83 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / b09a03c
          Default Java 1.8.0_101
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/16770/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16770/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16770/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 22s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 7m 56s trunk passed +1 compile 0m 49s trunk passed +1 checkstyle 0m 30s trunk passed +1 mvnsite 0m 57s trunk passed +1 mvneclipse 0m 15s trunk passed +1 findbugs 1m 47s trunk passed +1 javadoc 0m 57s trunk passed +1 mvninstall 0m 50s the patch passed +1 compile 0m 44s the patch passed +1 javac 0m 44s the patch passed +1 checkstyle 0m 25s the patch passed +1 mvnsite 0m 48s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 47s the patch passed +1 javadoc 0m 55s the patch passed -1 unit 88m 23s hadoop-hdfs in the patch failed. +1 asflicense 0m 30s The patch does not generate ASF License warnings. 109m 34s Reason Tests Failed junit tests hadoop.hdfs.server.datanode.TestDataNodeUUID   hadoop.hdfs.TestCrcCorruption   hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure Timed out junit tests org.apache.hadoop.hdfs.TestLeaseRecovery2 Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-10797 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828842/HDFS-10797.003.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 31121d442c83 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / b09a03c Default Java 1.8.0_101 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/16770/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16770/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16770/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Sean for revving! Patch 3 looks great to me.

          Hi Yongjun Zhang and Jing Zhao, could you please take a final look since you're more familiar with snapshots? Thanks in advance.

          Show
          xiaochen Xiao Chen added a comment - Thanks Sean for revving! Patch 3 looks great to me. Hi Yongjun Zhang and Jing Zhao , could you please take a final look since you're more familiar with snapshots? Thanks in advance.
          Hide
          xiaochen Xiao Chen added a comment -

          I took another look of the patch, +1.

          Yongjun Zhang Jing Zhao, do you have any comments? I plan to commit this by next Tuesday if no objections. Thanks.

          Show
          xiaochen Xiao Chen added a comment - I took another look of the patch, +1. Yongjun Zhang Jing Zhao , do you have any comments? I plan to commit this by next Tuesday if no objections. Thanks.
          Hide
          jingzhao Jing Zhao added a comment -

          Thanks for working on this, Sean. This is actually a known issue and the reason we currently choose to double the blocks is to make the semantic consistent with the scenario that the rename op moves the file out the original parent directory (e.g., into another snapshottable sub-tree or even a non-snapshottable subtree). In the later case, the moved files have to be counted in both the source directory and the target directory, since it belongs to snapshots of the source, and the current target directory.

          The scenario described in this jira is a special case. I agree it's strange to find that a file is counted twice after renaming under the same directory. However, with the current change the semantic will become inconsistent. For e.g., we will sometimes count the file twice and sometimes only once in the following scenario:
          1. move the file out of the original parent directory
          2. optional: take a new snapshot
          3. move the file back

          Without step 2 then the file is counted only once, but with step 2 since the newly created item is in another snapshot diff the file will be counted twice. Or if the file is renamed into a subdirectory of its parent directory (/foo/bar --> /foo/subdir/bar), this file is still double counted. From the end user point of view this inconsistency is also strange.

          So currently I'm leaning towards always doing double count. Thoughts?

          Show
          jingzhao Jing Zhao added a comment - Thanks for working on this, Sean. This is actually a known issue and the reason we currently choose to double the blocks is to make the semantic consistent with the scenario that the rename op moves the file out the original parent directory (e.g., into another snapshottable sub-tree or even a non-snapshottable subtree). In the later case, the moved files have to be counted in both the source directory and the target directory, since it belongs to snapshots of the source, and the current target directory. The scenario described in this jira is a special case. I agree it's strange to find that a file is counted twice after renaming under the same directory. However, with the current change the semantic will become inconsistent. For e.g., we will sometimes count the file twice and sometimes only once in the following scenario: 1. move the file out of the original parent directory 2. optional: take a new snapshot 3. move the file back Without step 2 then the file is counted only once, but with step 2 since the newly created item is in another snapshot diff the file will be counted twice. Or if the file is renamed into a subdirectory of its parent directory (/foo/bar --> /foo/subdir/bar), this file is still double counted. From the end user point of view this inconsistency is also strange. So currently I'm leaning towards always doing double count. Thoughts?
          Hide
          mackrorysd Sean Mackrory added a comment -

          Thanks for pointing that out Jing Zhao. I added test cases to address some inter-directory renames. Of course, some of them are broken and still reported the wrong usage. I'd really like to come up with a way for the semantics to be both consistent and unsurprising to a user. I improved the situation somewhat by computing which nodes were deleted (as opposed to renames) in the context of all the diffs for a directory instead of each diff individually. So it's a step in the right direction but the real fix would be to have some global context when computing usage that ensures each INode in the hierarchy is counted exactly once. It looks to me like that's going to require some refactoring, since although the counts are cumulative, they can accumulate in multiple distinct objects before being combined. We would need to refactor some functions that so all counts were added directly to a single object, and that same object could prevent nodes from being counted twice, once because they were removed from a snapshotted directory, and again because of where they reside now.

          Thoughts on this approach before I go further?

          Show
          mackrorysd Sean Mackrory added a comment - Thanks for pointing that out Jing Zhao . I added test cases to address some inter-directory renames. Of course, some of them are broken and still reported the wrong usage. I'd really like to come up with a way for the semantics to be both consistent and unsurprising to a user. I improved the situation somewhat by computing which nodes were deleted (as opposed to renames) in the context of all the diffs for a directory instead of each diff individually. So it's a step in the right direction but the real fix would be to have some global context when computing usage that ensures each INode in the hierarchy is counted exactly once. It looks to me like that's going to require some refactoring, since although the counts are cumulative, they can accumulate in multiple distinct objects before being combined. We would need to refactor some functions that so all counts were added directly to a single object, and that same object could prevent nodes from being counted twice, once because they were removed from a snapshotted directory, and again because of where they reside now. Thoughts on this approach before I go further?
          Hide
          jingzhao Jing Zhao added a comment -

          Sean Mackrory, I agree it will be great to have a consistent and user-friendly semantic. To me a better semantic can be like this: if the renamed source (which is inside of some snapshot) and the renamed target are both under the same directory for counting, we count them once. Otherwise they will be counted separately.

          With this semantic maybe we only need to move your hashset to the context object passed from the beginning of the counting call, and use it to avoid duplicated counting. What do you think?

          Show
          jingzhao Jing Zhao added a comment - Sean Mackrory , I agree it will be great to have a consistent and user-friendly semantic. To me a better semantic can be like this: if the renamed source (which is inside of some snapshot) and the renamed target are both under the same directory for counting, we count them once. Otherwise they will be counted separately. With this semantic maybe we only need to move your hashset to the context object passed from the beginning of the counting call, and use it to avoid duplicated counting. What do you think?
          Hide
          mackrorysd Sean Mackrory added a comment -

          Attaching the patch I meant to attach yesterday: this adds test cases for various inter-directory rename cases. In this patch, some of the test cases (specifically, the ones that check the space used by multiple directories) are expecting incorrect output. Also moved my logic outside of the loop so it applied to a large scope, and indeed - more of the test cases were now behaving correctly, but still not 100% correctly.

          Show
          mackrorysd Sean Mackrory added a comment - Attaching the patch I meant to attach yesterday: this adds test cases for various inter-directory rename cases. In this patch, some of the test cases (specifically, the ones that check the space used by multiple directories) are expecting incorrect output. Also moved my logic outside of the loop so it applied to a large scope, and indeed - more of the test cases were now behaving correctly, but still not 100% correctly.
          Hide
          mackrorysd Sean Mackrory added a comment - - edited

          Now attaching a patch that should give what I think we agree are the completely correct semantics:

          • du -s in a given directory will yield the space consumed by all snapshots and current files in the relevant directory, or below, regardless of subsequent renames or deletions.
          • du -s in a parent directory may yield a smaller value than the sum of all du -s results in child directories, if files have been snapshotted in one child directory and then move to another. There is overlap in the space consumed by each directory in this case.
          • In no cases is any INode counted twice in the context of a single du -s computation.

          In my opinion, these semantics are the most correct and least surprising to users, and they are consistent. If I understand your first reply correctly, I think you would agree with this, Jing Zhao? And the implementation resolves the inconsistency we were trying to avoid. Let me know if I've misunderstood anything...

          Attaching the patch, but going to do a little more manual testing, tinkering and thinking about this before "submitting" again since this is a very different approach from my previous patches.

          edit:

          To me a better semantic can be like this: if the renamed source (which is inside of some snapshot) and the renamed target are both under the same directory for counting, we count them once. Otherwise they will be counted separately.

          So I think what you're describing is what I ended up with in the .004 patch. I do think that's a step in the right direction, but still a little surprising and non-intuitive for someone who hasn't read our definition of the semantics carefully. I think .005 yields the ideal semantics I was originally going for. Do you agree?

          Show
          mackrorysd Sean Mackrory added a comment - - edited Now attaching a patch that should give what I think we agree are the completely correct semantics: du -s in a given directory will yield the space consumed by all snapshots and current files in the relevant directory, or below, regardless of subsequent renames or deletions. du -s in a parent directory may yield a smaller value than the sum of all du -s results in child directories, if files have been snapshotted in one child directory and then move to another. There is overlap in the space consumed by each directory in this case. In no cases is any INode counted twice in the context of a single du -s computation. In my opinion, these semantics are the most correct and least surprising to users, and they are consistent. If I understand your first reply correctly, I think you would agree with this, Jing Zhao ? And the implementation resolves the inconsistency we were trying to avoid. Let me know if I've misunderstood anything... Attaching the patch, but going to do a little more manual testing, tinkering and thinking about this before "submitting" again since this is a very different approach from my previous patches. edit: To me a better semantic can be like this: if the renamed source (which is inside of some snapshot) and the renamed target are both under the same directory for counting, we count them once. Otherwise they will be counted separately. So I think what you're describing is what I ended up with in the .004 patch. I do think that's a step in the right direction, but still a little surprising and non-intuitive for someone who hasn't read our definition of the semantics carefully. I think .005 yields the ideal semantics I was originally going for. Do you agree?
          Hide
          jingzhao Jing Zhao added a comment -

          Actually my proposal is like your .005 patch. The current semantic and approach looks good to me overall.

          Show
          jingzhao Jing Zhao added a comment - Actually my proposal is like your .005 patch. The current semantic and approach looks good to me overall.
          Hide
          mackrorysd Sean Mackrory added a comment -

          Great! During manual testing, I found a bug where if you snapshot a file, append to it and then rename it, space consumed suddenly decreases: the reference to the "deleted" node in the snapshot diff was getting counted first, and the current state of the file was then ignored. I changed the approach so summarization of any deleted nodes is deferred until the end - then we can choose whether or not to include them only once we have all the context we need. Also added a test case that would've caught the bug I found and cleaned up the code a bunch.

          Show
          mackrorysd Sean Mackrory added a comment - Great! During manual testing, I found a bug where if you snapshot a file, append to it and then rename it, space consumed suddenly decreases: the reference to the "deleted" node in the snapshot diff was getting counted first, and the current state of the file was then ignored. I changed the approach so summarization of any deleted nodes is deferred until the end - then we can choose whether or not to include them only once we have all the context we need. Also added a test case that would've caught the bug I found and cleaned up the code a bunch.
          Hide
          xiaochen Xiao Chen added a comment -

          Thank you for the good discussions here Sean Mackrory and Jing Zhao! Sorry I missed the scenario Jing mentioned. The semantic looks great to me.
          Some nits:

          • There's an used var count in INodeDirectory#computeContentSummary
          • ContentSummaryComputationContext#nodeIncluded 's java doc has some typos: both either

          And more importantly, I think some update on the INodeDirectory#computeContentSummary logic: the snapshotCounts added by HDFS-8986 is supposed to count only contents under snapshots. Looks like this change break the unit test from that jira. I think the subtreeSummary is the problem here: we should only add the snapshot portion of the subtree into snapshotCounts.

          Example unit test is TestDFSShell#testDuSnapshots. What do you guys think?

          Show
          xiaochen Xiao Chen added a comment - Thank you for the good discussions here Sean Mackrory and Jing Zhao ! Sorry I missed the scenario Jing mentioned. The semantic looks great to me. Some nits: There's an used var count in INodeDirectory#computeContentSummary ContentSummaryComputationContext#nodeIncluded 's java doc has some typos: both either And more importantly, I think some update on the INodeDirectory#computeContentSummary logic: the snapshotCounts added by HDFS-8986 is supposed to count only contents under snapshots. Looks like this change break the unit test from that jira. I think the subtreeSummary is the problem here: we should only add the snapshot portion of the subtree into snapshotCounts. Example unit test is TestDFSShell#testDuSnapshots . What do you guys think?
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 27s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 10m 8s trunk passed
          +1 compile 0m 54s trunk passed
          +1 checkstyle 0m 33s trunk passed
          +1 mvnsite 0m 55s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 48s trunk passed
          +1 javadoc 0m 56s trunk passed
          +1 mvninstall 0m 46s the patch passed
          +1 compile 0m 41s the patch passed
          +1 javac 0m 41s the patch passed
          -0 checkstyle 0m 26s hadoop-hdfs-project/hadoop-hdfs: The patch generated 3 new + 267 unchanged - 0 fixed = 270 total (was 267)
          +1 mvnsite 0m 52s the patch passed
          +1 mvneclipse 0m 9s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 48s the patch passed
          +1 javadoc 0m 53s the patch passed
          -1 unit 62m 47s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 22s The patch does not generate ASF License warnings.
          85m 54s



          Reason Tests
          Failed junit tests hadoop.hdfs.TestCrcCorruption
            hadoop.hdfs.TestDFSShell
            hadoop.hdfs.server.datanode.TestDirectoryScanner



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HDFS-10797
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830965/HDFS-10797.006.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 2e30f32b8da2 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 10be459
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/16933/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/16933/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16933/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16933/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 27s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 10m 8s trunk passed +1 compile 0m 54s trunk passed +1 checkstyle 0m 33s trunk passed +1 mvnsite 0m 55s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 48s trunk passed +1 javadoc 0m 56s trunk passed +1 mvninstall 0m 46s the patch passed +1 compile 0m 41s the patch passed +1 javac 0m 41s the patch passed -0 checkstyle 0m 26s hadoop-hdfs-project/hadoop-hdfs: The patch generated 3 new + 267 unchanged - 0 fixed = 270 total (was 267) +1 mvnsite 0m 52s the patch passed +1 mvneclipse 0m 9s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 48s the patch passed +1 javadoc 0m 53s the patch passed -1 unit 62m 47s hadoop-hdfs in the patch failed. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 85m 54s Reason Tests Failed junit tests hadoop.hdfs.TestCrcCorruption   hadoop.hdfs.TestDFSShell   hadoop.hdfs.server.datanode.TestDirectoryScanner Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-10797 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830965/HDFS-10797.006.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 2e30f32b8da2 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 10be459 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/16933/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/16933/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16933/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16933/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          mackrorysd Sean Mackrory added a comment -

          Thanks, Xiao Chen. Removed the count variable and fixed the Javadoc. I can actually remove a lot of code from INodeDirectory since it's no longer doing any actual counting in that loop - it's just identifying INodes that might be deleted and need to be looked at at the end, so we don't need to be swapping around counts objects there. Where that's now being done is in ContentSummaryComputationContext#tallyDeletedSnapshottedINodes, and there I was adding the counts from deletedNodes and add it to counts, and taking snapshotCounts from those nodes and adding them to snapshotCounts. As you point out, I should have added counts to both, like INodeDirectory was doing before. TestDFSShell and TestRenamesWithSnapshots now both working...

          Show
          mackrorysd Sean Mackrory added a comment - Thanks, Xiao Chen . Removed the count variable and fixed the Javadoc. I can actually remove a lot of code from INodeDirectory since it's no longer doing any actual counting in that loop - it's just identifying INodes that might be deleted and need to be looked at at the end, so we don't need to be swapping around counts objects there. Where that's now being done is in ContentSummaryComputationContext#tallyDeletedSnapshottedINodes, and there I was adding the counts from deletedNodes and add it to counts, and taking snapshotCounts from those nodes and adding them to snapshotCounts. As you point out, I should have added counts to both, like INodeDirectory was doing before. TestDFSShell and TestRenamesWithSnapshots now both working...
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 16s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 8m 27s trunk passed
          +1 compile 0m 51s trunk passed
          +1 checkstyle 0m 32s trunk passed
          +1 mvnsite 1m 3s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 56s trunk passed
          +1 javadoc 1m 1s trunk passed
          +1 mvninstall 0m 58s the patch passed
          +1 compile 0m 52s the patch passed
          +1 javac 0m 52s the patch passed
          -0 checkstyle 0m 28s hadoop-hdfs-project/hadoop-hdfs: The patch generated 3 new + 267 unchanged - 0 fixed = 270 total (was 267)
          +1 mvnsite 0m 56s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          -1 findbugs 1m 58s hadoop-hdfs-project/hadoop-hdfs generated 2 new + 0 unchanged - 0 fixed = 2 total (was 0)
          +1 javadoc 0m 58s the patch passed
          +1 unit 62m 31s hadoop-hdfs in the patch passed.
          +1 asflicense 0m 22s The patch does not generate ASF License warnings.
          84m 51s



          Reason Tests
          FindBugs module:hadoop-hdfs-project/hadoop-hdfs
            Synchronization on ContentSummaryComputationContext.counts in futile attempt to guard it At ContentSummaryComputationContext.java:attempt to guard it At ContentSummaryComputationContext.java:[line 224]
            Synchronization on ContentSummaryComputationContext.counts in futile attempt to guard it At ContentSummaryComputationContext.java:attempt to guard it At ContentSummaryComputationContext.java:[line 232]



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HDFS-10797
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12831131/HDFS-10797.007.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 27f500bb7b8c 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 0670149
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/16948/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
          findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/16948/artifact/patchprocess/new-findbugs-hadoop-hdfs-project_hadoop-hdfs.html
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16948/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16948/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 16s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 8m 27s trunk passed +1 compile 0m 51s trunk passed +1 checkstyle 0m 32s trunk passed +1 mvnsite 1m 3s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 56s trunk passed +1 javadoc 1m 1s trunk passed +1 mvninstall 0m 58s the patch passed +1 compile 0m 52s the patch passed +1 javac 0m 52s the patch passed -0 checkstyle 0m 28s hadoop-hdfs-project/hadoop-hdfs: The patch generated 3 new + 267 unchanged - 0 fixed = 270 total (was 267) +1 mvnsite 0m 56s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. -1 findbugs 1m 58s hadoop-hdfs-project/hadoop-hdfs generated 2 new + 0 unchanged - 0 fixed = 2 total (was 0) +1 javadoc 0m 58s the patch passed +1 unit 62m 31s hadoop-hdfs in the patch passed. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 84m 51s Reason Tests FindBugs module:hadoop-hdfs-project/hadoop-hdfs   Synchronization on ContentSummaryComputationContext.counts in futile attempt to guard it At ContentSummaryComputationContext.java:attempt to guard it At ContentSummaryComputationContext.java: [line 224]   Synchronization on ContentSummaryComputationContext.counts in futile attempt to guard it At ContentSummaryComputationContext.java:attempt to guard it At ContentSummaryComputationContext.java: [line 232] Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-10797 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12831131/HDFS-10797.007.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 27f500bb7b8c 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 0670149 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/16948/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/16948/artifact/patchprocess/new-findbugs-hadoop-hdfs-project_hadoop-hdfs.html Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16948/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16948/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          mackrorysd Sean Mackrory added a comment -

          findbugs output is correct: I'm using synchronization to prevent any problems caused by the fact that we swap the counts object to tally up deleted, snapshotted space. We did this before, and it should be fine, since this is single-threaded everywhere I've seen, but thought it'd be wise to guard just in case. I'll synchronize the entire function on the context object instead and resubmit - other feedback welcome in the mean time, since that won't affect most of the patch.

          Show
          mackrorysd Sean Mackrory added a comment - findbugs output is correct: I'm using synchronization to prevent any problems caused by the fact that we swap the counts object to tally up deleted, snapshotted space. We did this before, and it should be fine, since this is single-threaded everywhere I've seen, but thought it'd be wise to guard just in case. I'll synchronize the entire function on the context object instead and resubmit - other feedback welcome in the mean time, since that won't affect most of the patch.
          Hide
          mackrorysd Sean Mackrory added a comment -

          Attaching a patch that address the findbugs warnings.

          Show
          mackrorysd Sean Mackrory added a comment - Attaching a patch that address the findbugs warnings.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 13s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 6m 59s trunk passed
          +1 compile 0m 46s trunk passed
          +1 checkstyle 0m 29s trunk passed
          +1 mvnsite 0m 52s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 46s trunk passed
          +1 javadoc 0m 56s trunk passed
          +1 mvninstall 0m 48s the patch passed
          +1 compile 0m 43s the patch passed
          +1 javac 0m 43s the patch passed
          -0 checkstyle 0m 26s hadoop-hdfs-project/hadoop-hdfs: The patch generated 3 new + 267 unchanged - 0 fixed = 270 total (was 267)
          +1 mvnsite 0m 53s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          -1 findbugs 1m 54s hadoop-hdfs-project/hadoop-hdfs generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
          +1 javadoc 0m 53s the patch passed
          -1 unit 58m 42s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 18s The patch does not generate ASF License warnings.
          78m 16s



          Reason Tests
          FindBugs module:hadoop-hdfs-project/hadoop-hdfs
            Inconsistent synchronization of org.apache.hadoop.hdfs.server.namenode.ContentSummaryComputationContext.counts; locked 55% of time Unsynchronized access at ContentSummaryComputationContext.java:55% of time Unsynchronized access at ContentSummaryComputationContext.java:[line 91]
          Failed junit tests hadoop.hdfs.TestDFSShell
            hadoop.hdfs.TestFileCorruption



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HDFS-10797
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12831164/HDFS-10797.008.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 7ddd29c9e58a 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 2549ee9
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/16953/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
          findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/16953/artifact/patchprocess/new-findbugs-hadoop-hdfs-project_hadoop-hdfs.html
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/16953/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16953/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16953/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 13s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 6m 59s trunk passed +1 compile 0m 46s trunk passed +1 checkstyle 0m 29s trunk passed +1 mvnsite 0m 52s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 46s trunk passed +1 javadoc 0m 56s trunk passed +1 mvninstall 0m 48s the patch passed +1 compile 0m 43s the patch passed +1 javac 0m 43s the patch passed -0 checkstyle 0m 26s hadoop-hdfs-project/hadoop-hdfs: The patch generated 3 new + 267 unchanged - 0 fixed = 270 total (was 267) +1 mvnsite 0m 53s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. -1 findbugs 1m 54s hadoop-hdfs-project/hadoop-hdfs generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) +1 javadoc 0m 53s the patch passed -1 unit 58m 42s hadoop-hdfs in the patch failed. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 78m 16s Reason Tests FindBugs module:hadoop-hdfs-project/hadoop-hdfs   Inconsistent synchronization of org.apache.hadoop.hdfs.server.namenode.ContentSummaryComputationContext.counts; locked 55% of time Unsynchronized access at ContentSummaryComputationContext.java:55% of time Unsynchronized access at ContentSummaryComputationContext.java: [line 91] Failed junit tests hadoop.hdfs.TestDFSShell   hadoop.hdfs.TestFileCorruption Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-10797 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12831164/HDFS-10797.008.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 7ddd29c9e58a 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 2549ee9 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/16953/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/16953/artifact/patchprocess/new-findbugs-hadoop-hdfs-project_hadoop-hdfs.html unit https://builds.apache.org/job/PreCommit-HDFS-Build/16953/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16953/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16953/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          mackrorysd Sean Mackrory added a comment -

          Fixing style issues, and always using getCounts() to retrieve counts for thread safety (which I noticed actually is important here, due to the yield() function).

          Show
          mackrorysd Sean Mackrory added a comment - Fixing style issues, and always using getCounts() to retrieve counts for thread safety (which I noticed actually is important here, due to the yield() function).
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 7m 37s trunk passed
          +1 compile 0m 49s trunk passed
          +1 checkstyle 0m 30s trunk passed
          +1 mvnsite 0m 57s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 49s trunk passed
          +1 javadoc 1m 3s trunk passed
          +1 mvninstall 0m 55s the patch passed
          +1 compile 0m 48s the patch passed
          +1 javac 0m 48s the patch passed
          -0 checkstyle 0m 28s hadoop-hdfs-project/hadoop-hdfs: The patch generated 1 new + 267 unchanged - 0 fixed = 268 total (was 267)
          +1 mvnsite 0m 55s the patch passed
          +1 mvneclipse 0m 11s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 2m 2s the patch passed
          +1 javadoc 0m 58s the patch passed
          -1 unit 61m 55s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 17s The patch does not generate ASF License warnings.
          83m 5s



          Reason Tests
          Failed junit tests hadoop.hdfs.server.blockmanagement.TestPendingInvalidateBlock
          Timed out junit tests org.apache.hadoop.hdfs.TestSmallBlock



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HDFS-10797
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12831199/HDFS-10797.009.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 2925a49ab866 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 2549ee9
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/16961/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/16961/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16961/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16961/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 17s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 7m 37s trunk passed +1 compile 0m 49s trunk passed +1 checkstyle 0m 30s trunk passed +1 mvnsite 0m 57s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 49s trunk passed +1 javadoc 1m 3s trunk passed +1 mvninstall 0m 55s the patch passed +1 compile 0m 48s the patch passed +1 javac 0m 48s the patch passed -0 checkstyle 0m 28s hadoop-hdfs-project/hadoop-hdfs: The patch generated 1 new + 267 unchanged - 0 fixed = 268 total (was 267) +1 mvnsite 0m 55s the patch passed +1 mvneclipse 0m 11s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 2m 2s the patch passed +1 javadoc 0m 58s the patch passed -1 unit 61m 55s hadoop-hdfs in the patch failed. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 83m 5s Reason Tests Failed junit tests hadoop.hdfs.server.blockmanagement.TestPendingInvalidateBlock Timed out junit tests org.apache.hadoop.hdfs.TestSmallBlock Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-10797 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12831199/HDFS-10797.009.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 2925a49ab866 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 2549ee9 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/16961/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/16961/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16961/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16961/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Sean for the new revs! I think the synchronized block is ok, as I don't think that will be a critical path to impact du performance. Not sure what Jing Zhao's take is on this one.

          Some minor comments, all in ContentSummaryComputationContext:

          • In nodeIncluded, we safeguard includedNodes in a synchronized block, but we also provide a getIncludedNodes method, which could potentially be updated by the caller. No real usage yet, but I just feel this a bit unsafe in general, maybe return a clone of it instead?
          • resolveINodeReference can be private
          • Need a <p> to separate paragraphs in javadoc.

          Test looks great.

          Show
          xiaochen Xiao Chen added a comment - Thanks Sean for the new revs! I think the synchronized block is ok, as I don't think that will be a critical path to impact du performance. Not sure what Jing Zhao 's take is on this one. Some minor comments, all in ContentSummaryComputationContext : In nodeIncluded , we safeguard includedNodes in a synchronized block, but we also provide a getIncludedNodes method, which could potentially be updated by the caller. No real usage yet, but I just feel this a bit unsafe in general, maybe return a clone of it instead? resolveINodeReference can be private Need a <p> to separate paragraphs in javadoc. Test looks great.
          Hide
          mackrorysd Sean Mackrory added a comment -

          Thanks, Xiao Chen. Except as noted below, I'll incorporate all your feedback into another patch...

          I don't think that will be a critical path to impact du performance

          Yeah - not sure if anything performance critical depends on du, but I would think correctness of the final result is far more important here anyway.

          In nodeIncluded, we safeguard includedNodes in a synchronized block, but we also provide a getIncludedNodes method, which could potentially be updated by the caller. No real usage yet, but I just feel this a bit unsafe in general, maybe return a clone of it instead?

          So my concern was not that the contents of the HashSet instance might change, but that the reference 'counts' temporarily points to a different object entirely when tallying the deleted, snapshotted INodes. Rather than protecting the data structures, it ensures no one can call getCounts() while counts would point to the wrong object. Beyond that, I think it's just as likely that threads calling getCounts in parallel will need their changes to propagate to the rest of the program, meaning the correct solution would be a thread-safe data structure rather than a clone. So I do think it's best to leave it as is until there is a use case for other concurrent accesses.

          Show
          mackrorysd Sean Mackrory added a comment - Thanks, Xiao Chen . Except as noted below, I'll incorporate all your feedback into another patch... I don't think that will be a critical path to impact du performance Yeah - not sure if anything performance critical depends on du, but I would think correctness of the final result is far more important here anyway. In nodeIncluded, we safeguard includedNodes in a synchronized block, but we also provide a getIncludedNodes method, which could potentially be updated by the caller. No real usage yet, but I just feel this a bit unsafe in general, maybe return a clone of it instead? So my concern was not that the contents of the HashSet instance might change, but that the reference 'counts' temporarily points to a different object entirely when tallying the deleted, snapshotted INodes. Rather than protecting the data structures, it ensures no one can call getCounts() while counts would point to the wrong object. Beyond that, I think it's just as likely that threads calling getCounts in parallel will need their changes to propagate to the rest of the program, meaning the correct solution would be a thread-safe data structure rather than a clone. So I do think it's best to leave it as is until there is a use case for other concurrent accesses.
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Sean. Agreed correctness is the goal. My comment regarding performance was on the nature of the context based summary, which is added to prevent a du locking the NN. And as we both agreed, this patch doesn't touch that.

          Also about getIncludedNodes, it seems we don't call it anywhere... Maybe we can remove the getter method, and when a use case emerges, let the new change handle it?

          Show
          xiaochen Xiao Chen added a comment - Thanks Sean. Agreed correctness is the goal. My comment regarding performance was on the nature of the context based summary, which is added to prevent a du locking the NN. And as we both agreed, this patch doesn't touch that. Also about getIncludedNodes , it seems we don't call it anywhere... Maybe we can remove the getter method, and when a use case emerges, let the new change handle it?
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 6m 56s trunk passed
          +1 compile 0m 44s trunk passed
          +1 checkstyle 0m 30s trunk passed
          +1 mvnsite 0m 52s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 1m 42s trunk passed
          +1 javadoc 0m 55s trunk passed
          +1 mvninstall 0m 46s the patch passed
          +1 compile 0m 41s the patch passed
          +1 javac 0m 41s the patch passed
          -0 checkstyle 0m 27s hadoop-hdfs-project/hadoop-hdfs: The patch generated 1 new + 267 unchanged - 0 fixed = 268 total (was 267)
          +1 mvnsite 0m 48s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 46s the patch passed
          +1 javadoc 0m 51s the patch passed
          -1 unit 57m 5s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 19s The patch does not generate ASF License warnings.
          76m 21s



          Reason Tests
          Failed junit tests hadoop.hdfs.server.balancer.TestBalancerWithMultipleNameNodes



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HDFS-10797
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12831783/HDFS-10797.010.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 13a2e115431f 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / d6be1e7
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17022/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/17022/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17022/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17022/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 17s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 6m 56s trunk passed +1 compile 0m 44s trunk passed +1 checkstyle 0m 30s trunk passed +1 mvnsite 0m 52s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 1m 42s trunk passed +1 javadoc 0m 55s trunk passed +1 mvninstall 0m 46s the patch passed +1 compile 0m 41s the patch passed +1 javac 0m 41s the patch passed -0 checkstyle 0m 27s hadoop-hdfs-project/hadoop-hdfs: The patch generated 1 new + 267 unchanged - 0 fixed = 268 total (was 267) +1 mvnsite 0m 48s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 46s the patch passed +1 javadoc 0m 51s the patch passed -1 unit 57m 5s hadoop-hdfs in the patch failed. +1 asflicense 0m 19s The patch does not generate ASF License warnings. 76m 21s Reason Tests Failed junit tests hadoop.hdfs.server.balancer.TestBalancerWithMultipleNameNodes Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-10797 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12831783/HDFS-10797.010.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 13a2e115431f 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / d6be1e7 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17022/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/17022/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17022/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17022/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks for revving Sean. +1 on patch 10.
          Jing Zhao do you have any comments?

          Show
          xiaochen Xiao Chen added a comment - Thanks for revving Sean. +1 on patch 10. Jing Zhao do you have any comments?
          Hide
          xiaochen Xiao Chen added a comment -

          I plan to commit this end of today if no objections.

          Show
          xiaochen Xiao Chen added a comment - I plan to commit this end of today if no objections.
          Hide
          xiaochen Xiao Chen added a comment -

          Committed this to trunk, branch-2 and branch-2.8. Thanks Sean for the great work here and Jing for the helpful reviews!

          I think this is just a bug fix, and the change in du output is considered incorrect previously. So not marking this incompatible.

          Sean Mackrory, do you mind adding a short release note to the jira? Thanks

          Show
          xiaochen Xiao Chen added a comment - Committed this to trunk, branch-2 and branch-2.8. Thanks Sean for the great work here and Jing for the helpful reviews! I think this is just a bug fix, and the change in du output is considered incorrect previously. So not marking this incompatible. Sean Mackrory , do you mind adding a short release note to the jira? Thanks
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10572 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10572/)
          HDFS-10797. Disk usage summary of snapshots causes renamed blocks to get (xiao: rev 6a38d118d86b7907009bcec34f1b788d076f1d1c)

          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeSymlink.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeFile.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INode.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithSnapshots.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeDirectory.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/Snapshot.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/DirectoryWithSnapshotFeature.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/DirectorySnapshottableFeature.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeReference.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ContentSummaryComputationContext.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10572 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10572/ ) HDFS-10797 . Disk usage summary of snapshots causes renamed blocks to get (xiao: rev 6a38d118d86b7907009bcec34f1b788d076f1d1c) (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeSymlink.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeFile.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INode.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithSnapshots.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeDirectory.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/Snapshot.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/DirectoryWithSnapshotFeature.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/DirectorySnapshottableFeature.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeReference.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ContentSummaryComputationContext.java
          Hide
          mackrorysd Sean Mackrory added a comment -

          Thanks Xiao Chen. I added a release note...

          Show
          mackrorysd Sean Mackrory added a comment - Thanks Xiao Chen . I added a release note...
          Hide
          nroberts Nathan Roberts added a comment -

          After deploying this to a cluster with a few hundred nodes, we have discovered that this jira has caused significant memory bloat in the namenode. Filed 2.8.1 blocker for this issue - HDFS-11661.

          Show
          nroberts Nathan Roberts added a comment - After deploying this to a cluster with a few hundred nodes, we have discovered that this jira has caused significant memory bloat in the namenode. Filed 2.8.1 blocker for this issue - HDFS-11661 .
          Hide
          djp Junping Du added a comment - - edited

          Hi Sean Mackrory and Xiao Chen, can you take a look at HDFS-11661 which could be caused by improvement here? Thx!

          Show
          djp Junping Du added a comment - - edited Hi Sean Mackrory and Xiao Chen , can you take a look at HDFS-11661 which could be caused by improvement here? Thx!
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11776 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11776/)
          Revert "HDFS-10797. Disk usage summary of snapshots causes renamed (weichiu: rev b8b69d797aed8dfeb65ea462c2856f62e9aa1023)

          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeFile.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeSymlink.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/DirectoryWithSnapshotFeature.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithSnapshots.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ContentSummaryComputationContext.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/DirectorySnapshottableFeature.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeDirectory.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeReference.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INode.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/Snapshot.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11776 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11776/ ) Revert " HDFS-10797 . Disk usage summary of snapshots causes renamed (weichiu: rev b8b69d797aed8dfeb65ea462c2856f62e9aa1023) (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeFile.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeSymlink.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/DirectoryWithSnapshotFeature.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithSnapshots.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ContentSummaryComputationContext.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/DirectorySnapshottableFeature.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeDirectory.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeReference.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INode.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/Snapshot.java

            People

            • Assignee:
              mackrorysd Sean Mackrory
              Reporter:
              mackrorysd Sean Mackrory
            • Votes:
              0 Vote for this issue
              Watchers:
              14 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development