Details

      Description

      Since the directory-level EC policy simply applies to files at create time, it makes sense to make it more similar to storage policies and allow changing and unsetting the policy.

      1. HDFS-11072-v1.patch
        16 kB
        SammiChen
      2. HDFS-11072-v2.patch
        21 kB
        SammiChen
      3. HDFS-11072-v3.patch
        27 kB
        SammiChen
      4. HDFS-11072-v4.patch
        27 kB
        SammiChen
      5. HDFS-11072-v5.patch
        40 kB
        SammiChen
      6. HDFS-11072-v6.patch
        43 kB
        SammiChen
      7. HDFS-11072-v7.patch
        43 kB
        SammiChen

        Issue Links

          Activity

          Hide
          andrew.wang Andrew Wang added a comment -

          Two other notes:

          • The documentation should also be revamped to explain how EC policy can be configured
          • We also need a "replicated" EC policy so we can override the EC policy on a parent directory and go back to just replicating the files
          Show
          andrew.wang Andrew Wang added a comment - Two other notes: The documentation should also be revamped to explain how EC policy can be configured We also need a "replicated" EC policy so we can override the EC policy on a parent directory and go back to just replicating the files
          Hide
          Sammi SammiChen added a comment -

          Hi Andrew Wang, thanks for fire this JIRA, I'd like to work on it. Here is what I'am understanding. Correct me if I do't follow your thoughts.
          Assume we create a directory A, set EC policy EC1 on A. then we create a sub directory B under A. When B is created, it will inherit EC policy EC1 from A, and save it as its own meta data. Later when directory A's EC policy EC1 is unset or changed to EC2, that would not impact directory B. Directory B will always has EC policy EC1 unless user explicitly change it. With this behavior, we'are going to support the interleaved EC policy directory and 3x replication directory.

          Show
          Sammi SammiChen added a comment - Hi Andrew Wang , thanks for fire this JIRA, I'd like to work on it. Here is what I'am understanding. Correct me if I do't follow your thoughts. Assume we create a directory A, set EC policy EC1 on A. then we create a sub directory B under A. When B is created, it will inherit EC policy EC1 from A, and save it as its own meta data. Later when directory A's EC policy EC1 is unset or changed to EC2, that would not impact directory B. Directory B will always has EC policy EC1 unless user explicitly change it. With this behavior, we'are going to support the interleaved EC policy directory and 3x replication directory.
          Hide
          andrew.wang Andrew Wang added a comment -

          Hi Sammi, your thinking is accurate. One little note though, we'd like to be able to be able to support arbitrary replication factors, not just 3x. I hope this is possible without too much extra work.

          Show
          andrew.wang Andrew Wang added a comment - Hi Sammi, your thinking is accurate. One little note though, we'd like to be able to be able to support arbitrary replication factors, not just 3x. I hope this is possible without too much extra work.
          Hide
          Sammi SammiChen added a comment -

          Agree, arbitrary replication factors should be supported. I have other thoughts for the inheritance behavior. If we create directory A, set policy EC1 on it, then create B under A, C under B.
          Later, if we want to change the whole directory A from EC1 to EC2, we have to recursively go through all directories under A, change it one by one. It sounds not very convenient. And since EC policy on a directory is either inherited from its parent or explicitly set by admin, we need a flag to indicate whether the policy is inherited or not. So here is my propose, for each directory, add a new Boolean flag, say inheritECPolicy. When the flag is true, then query this directory's EC policy should look through its parent's EC policy. When the flag is false, then the EC policy stored with this directory takes effect.

          Show
          Sammi SammiChen added a comment - Agree, arbitrary replication factors should be supported. I have other thoughts for the inheritance behavior. If we create directory A, set policy EC1 on it, then create B under A, C under B. Later, if we want to change the whole directory A from EC1 to EC2, we have to recursively go through all directories under A, change it one by one. It sounds not very convenient. And since EC policy on a directory is either inherited from its parent or explicitly set by admin, we need a flag to indicate whether the policy is inherited or not. So here is my propose, for each directory, add a new Boolean flag, say inheritECPolicy . When the flag is true, then query this directory's EC policy should look through its parent's EC policy. When the flag is false, then the EC policy stored with this directory takes effect.
          Hide
          rakeshr Rakesh R added a comment -

          Thanks SammiChen, Andrew Wang for the good work. Just few thoughts.

          Agree, arbitrary replication factors should be supported

          From the jira subjectline I understand that the idea is to support unset/change EC policy only on a directory and not on the file. Also, I think you are not planning to touch(ec to non-ec while unsetting policy) any of the existing files under this directory on performing this operation. In that case, do we need to care about the replication factor. IIUC, the directory doesn't have the replication factor. Am I missing anything?

          Later, if we want to change the whole directory A from EC1 to EC2, we have to recursively go through all directories under A, change it one by one. It sounds not very convenient.

          Yes, imho, its not required to recursively go through all directories under a directory and apply unset/change policy. Probably we could make this function call simple and apply the changes only to the given directory. If user wants, then let them iterate and call this function multiple times.

          And since EC policy on a directory is either inherited from its parent or explicitly set by admin, we need a flag to indicate whether the policy is inherited or not

          I'm interested to know, why do we differentiate that the policy is inherited from its parent or explicitly set by admin.

          Show
          rakeshr Rakesh R added a comment - Thanks SammiChen , Andrew Wang for the good work. Just few thoughts. Agree, arbitrary replication factors should be supported From the jira subjectline I understand that the idea is to support unset/change EC policy only on a directory and not on the file. Also, I think you are not planning to touch(ec to non-ec while unsetting policy) any of the existing files under this directory on performing this operation. In that case, do we need to care about the replication factor. IIUC, the directory doesn't have the replication factor. Am I missing anything? Later, if we want to change the whole directory A from EC1 to EC2, we have to recursively go through all directories under A, change it one by one. It sounds not very convenient. Yes, imho, its not required to recursively go through all directories under a directory and apply unset/change policy. Probably we could make this function call simple and apply the changes only to the given directory. If user wants, then let them iterate and call this function multiple times. And since EC policy on a directory is either inherited from its parent or explicitly set by admin, we need a flag to indicate whether the policy is inherited or not I'm interested to know, why do we differentiate that the policy is inherited from its parent or explicitly set by admin.
          Hide
          rakeshr Rakesh R added a comment -

          We also need a "replicated" EC policy so we can override the EC policy on a parent directory and go back to just replicating the files

          Hi Andrew Wang, Instead of adding new "replicated" EC policy, how about call unset EC policy on a directory and the new files under this will now start writing in replicated fashion. Like you mentioned in HDFS-10996, a conversion tool will take care of converting all existing ec files to replicated one and vice versa. Its just a thought, please feel free to correct me if I missed anything. Thanks!

          Show
          rakeshr Rakesh R added a comment - We also need a "replicated" EC policy so we can override the EC policy on a parent directory and go back to just replicating the files Hi Andrew Wang , Instead of adding new "replicated" EC policy, how about call unset EC policy on a directory and the new files under this will now start writing in replicated fashion. Like you mentioned in HDFS-10996, a conversion tool will take care of converting all existing ec files to replicated one and vice versa. Its just a thought, please feel free to correct me if I missed anything. Thanks!
          Hide
          andrew.wang Andrew Wang added a comment -

          IIUC EC policy is implemented by traversing the target path backwards until we find a directory with a policy set. This is how storage policies work at least. Reusing Sammi's example:

          mkdir -p /A/B/C
          erasurecode -setPolicy thepolicy /A
          

          Files created under A, B, or C all will inherit the policy set on A. If we wanted to change the policy, we just call -setPolicy again on A and it will be picked up by new files. Basically, the default policy is to inherit from the parent.

          In that case, do we need to care about the replication factor. IIUC, the directory doesn't have the replication factor. Am I missing anything?

          You're correct, there isn't a replication factor specified. I just wanted to clarify with Sammi earlier, since there was a mention of "3x replicated" specifically, when really it can be general.

          Probably we could make this function call simple and apply the changes only to the given directory. If user wants, then let them iterate and call this function multiple times.

          +1. It might also be nice to provide a CLI tool that prints the policies on the parent directories of a path.

          bq, Instead of adding new "replicated" EC policy, how about call unset EC policy on a directory and the new files under this will now start writing in replicated fashion.

          Hopefully the above example clarified this, but basically if we want a subdir of a directory with an EC policy (e.g. /A/B/R) to create replicated files, we need to set a policy that's not the default "inherit from parent" policy.

          Show
          andrew.wang Andrew Wang added a comment - IIUC EC policy is implemented by traversing the target path backwards until we find a directory with a policy set. This is how storage policies work at least. Reusing Sammi's example: mkdir -p /A/B/C erasurecode -setPolicy thepolicy /A Files created under A, B, or C all will inherit the policy set on A. If we wanted to change the policy, we just call -setPolicy again on A and it will be picked up by new files. Basically, the default policy is to inherit from the parent. In that case, do we need to care about the replication factor. IIUC, the directory doesn't have the replication factor. Am I missing anything? You're correct, there isn't a replication factor specified. I just wanted to clarify with Sammi earlier, since there was a mention of "3x replicated" specifically, when really it can be general. Probably we could make this function call simple and apply the changes only to the given directory. If user wants, then let them iterate and call this function multiple times. +1. It might also be nice to provide a CLI tool that prints the policies on the parent directories of a path. bq, Instead of adding new "replicated" EC policy, how about call unset EC policy on a directory and the new files under this will now start writing in replicated fashion. Hopefully the above example clarified this, but basically if we want a subdir of a directory with an EC policy (e.g. /A/B/R ) to create replicated files, we need to set a policy that's not the default "inherit from parent" policy.
          Hide
          rakeshr Rakesh R added a comment -

          Thanks a lot Andrew Wang for the detailed explanation.

          It might also be nice to provide a CLI tool that prints the policies on the parent directories of a path.

          This is really an interesting thought. I hope your idea is that, users can easily find the difference in EC policies between the given path and its parent directory path. If I take the above example, for the given /A/B/R path, say R has "replicated" EC policy and its parent /A/B has "rs-default" policy.

          Hopefully the above example clarified this, but basically if we want a subdir of a directory with an EC policy (e.g. /A/B/R) to create replicated files, we need to set a policy that's not the default "inherit from parent" policy.

          Yes, its a good case. I understand the need of "replicated" EC policy. I hope this can be done via separate task and there is no jira raised for this. I'm happy to start thinking about the implementation, shall I raise a jira for this?

          Show
          rakeshr Rakesh R added a comment - Thanks a lot Andrew Wang for the detailed explanation. It might also be nice to provide a CLI tool that prints the policies on the parent directories of a path. This is really an interesting thought. I hope your idea is that, users can easily find the difference in EC policies between the given path and its parent directory path. If I take the above example, for the given /A/B/R path, say R has "replicated" EC policy and its parent /A/B has "rs-default" policy. Hopefully the above example clarified this, but basically if we want a subdir of a directory with an EC policy (e.g. /A/B/R) to create replicated files, we need to set a policy that's not the default "inherit from parent" policy. Yes, its a good case. I understand the need of "replicated" EC policy. I hope this can be done via separate task and there is no jira raised for this. I'm happy to start thinking about the implementation, shall I raise a jira for this?
          Hide
          andrew.wang Andrew Wang added a comment -

          New JIRA(s) sound great! Thanks for volunteering Rakesh R.

          Show
          andrew.wang Andrew Wang added a comment - New JIRA(s) sound great! Thanks for volunteering Rakesh R .
          Hide
          Sammi SammiChen added a comment -

          Initial patch!

          Show
          Sammi SammiChen added a comment - Initial patch!
          Hide
          Sammi SammiChen added a comment -

          Thanks Andrew Wang and Rakesh R for your discussion which gives me more thoughts on the task. Here is the summary of what have done in the initial patch.
          1. define one special "continuous replicate" policy which is used to set a directory from erasure coding policy back to default replicates, or set a directory from stop inheriting its parent directory's erasure coding policy. This special "continuous replicate" policy is only used when setErasureCodingPolicy function is called. It will never be returned when getErasureCodingPolicy with a path parameter is called, to avoid messing other erasure coding codes.
          2 user is allowed to do "replicates <=> ec policy" and "one ec policy <=> another ec policy" change on a directory.
          3. one thing implemented which is different from what we have discussed before. Let use an example, If user create /A/B/C, set ecPolicy1 on "A", then all new created files(direct and indirect) under "A" will adopt ecPolicy1. And at some point, user set ecPolicy2 on "A". After that, all new created files(both direct and indirect) under "A" will adopt ecPolicy2.
          4. add test case

          Show
          Sammi SammiChen added a comment - Thanks Andrew Wang and Rakesh R for your discussion which gives me more thoughts on the task. Here is the summary of what have done in the initial patch. 1. define one special "continuous replicate" policy which is used to set a directory from erasure coding policy back to default replicates, or set a directory from stop inheriting its parent directory's erasure coding policy. This special "continuous replicate" policy is only used when setErasureCodingPolicy function is called. It will never be returned when getErasureCodingPolicy with a path parameter is called, to avoid messing other erasure coding codes. 2 user is allowed to do "replicates <=> ec policy" and "one ec policy <=> another ec policy" change on a directory. 3. one thing implemented which is different from what we have discussed before. Let use an example, If user create /A/B/C, set ecPolicy1 on "A", then all new created files(direct and indirect) under "A" will adopt ecPolicy1. And at some point, user set ecPolicy2 on "A". After that, all new created files(both direct and indirect) under "A" will adopt ecPolicy2. 4. add test case
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s 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.
          0 mvndep 1m 47s Maven dependency ordering for branch
          +1 mvninstall 7m 6s trunk passed
          +1 compile 9m 35s trunk passed
          +1 checkstyle 1m 31s trunk passed
          +1 mvnsite 2m 38s trunk passed
          +1 mvneclipse 0m 53s trunk passed
          +1 findbugs 4m 40s trunk passed
          +1 javadoc 1m 55s trunk passed
          0 mvndep 0m 15s Maven dependency ordering for patch
          +1 mvninstall 1m 51s the patch passed
          +1 compile 9m 7s the patch passed
          +1 javac 9m 7s the patch passed
          -0 checkstyle 1m 32s root: The patch generated 3 new + 10 unchanged - 0 fixed = 13 total (was 10)
          +1 mvnsite 2m 34s the patch passed
          +1 mvneclipse 0m 54s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 5m 8s the patch passed
          +1 javadoc 1m 58s the patch passed
          +1 unit 7m 31s hadoop-common in the patch passed.
          +1 unit 1m 2s hadoop-hdfs-client in the patch passed.
          -1 unit 63m 4s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 34s The patch does not generate ASF License warnings.
          127m 14s



          Reason Tests
          Failed junit tests hadoop.hdfs.TestErasureCodingPolicies
            hadoop.hdfs.server.datanode.TestBlockRecovery
            hadoop.hdfs.util.TestStripedBlockUtil
            hadoop.cli.TestErasureCodingCLI
            hadoop.hdfs.server.namenode.TestStripedINodeFile



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11072
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12840630/HDFS-11072-v1.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 9af49943019e 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 5d5614f
          Default Java 1.8.0_111
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17672/artifact/patchprocess/diff-checkstyle-root.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/17672/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17672/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17672/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 15s 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. 0 mvndep 1m 47s Maven dependency ordering for branch +1 mvninstall 7m 6s trunk passed +1 compile 9m 35s trunk passed +1 checkstyle 1m 31s trunk passed +1 mvnsite 2m 38s trunk passed +1 mvneclipse 0m 53s trunk passed +1 findbugs 4m 40s trunk passed +1 javadoc 1m 55s trunk passed 0 mvndep 0m 15s Maven dependency ordering for patch +1 mvninstall 1m 51s the patch passed +1 compile 9m 7s the patch passed +1 javac 9m 7s the patch passed -0 checkstyle 1m 32s root: The patch generated 3 new + 10 unchanged - 0 fixed = 13 total (was 10) +1 mvnsite 2m 34s the patch passed +1 mvneclipse 0m 54s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 5m 8s the patch passed +1 javadoc 1m 58s the patch passed +1 unit 7m 31s hadoop-common in the patch passed. +1 unit 1m 2s hadoop-hdfs-client in the patch passed. -1 unit 63m 4s hadoop-hdfs in the patch failed. +1 asflicense 0m 34s The patch does not generate ASF License warnings. 127m 14s Reason Tests Failed junit tests hadoop.hdfs.TestErasureCodingPolicies   hadoop.hdfs.server.datanode.TestBlockRecovery   hadoop.hdfs.util.TestStripedBlockUtil   hadoop.cli.TestErasureCodingCLI   hadoop.hdfs.server.namenode.TestStripedINodeFile Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11072 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12840630/HDFS-11072-v1.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 9af49943019e 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 5d5614f Default Java 1.8.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/17672/artifact/patchprocess/diff-checkstyle-root.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/17672/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17672/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17672/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          Sammi SammiChen added a comment -

          1. fix style issues
          2. add more test case
          3. solve failed test case

          Show
          Sammi SammiChen added a comment - 1. fix style issues 2. add more test case 3. solve failed test case
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 19s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          0 mvndep 1m 41s Maven dependency ordering for branch
          +1 mvninstall 8m 27s trunk passed
          +1 compile 11m 25s trunk passed
          +1 checkstyle 1m 36s trunk passed
          +1 mvnsite 2m 44s trunk passed
          +1 mvneclipse 0m 57s trunk passed
          +1 findbugs 5m 2s trunk passed
          +1 javadoc 2m 11s trunk passed
          0 mvndep 0m 18s Maven dependency ordering for patch
          +1 mvninstall 2m 7s the patch passed
          +1 compile 9m 42s the patch passed
          +1 javac 9m 42s the patch passed
          +1 checkstyle 1m 38s the patch passed
          +1 mvnsite 2m 56s the patch passed
          +1 mvneclipse 1m 3s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 5m 21s the patch passed
          +1 javadoc 2m 1s the patch passed
          +1 unit 9m 26s hadoop-common in the patch passed.
          +1 unit 1m 1s hadoop-hdfs-client in the patch passed.
          -1 unit 95m 56s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 35s The patch does not generate ASF License warnings.
          167m 53s



          Reason Tests
          Failed junit tests hadoop.hdfs.TestDFSStripedOutputStreamWithFailure120
            hadoop.cli.TestErasureCodingCLI
            hadoop.hdfs.TestRollingUpgrade



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11072
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12840651/HDFS-11072-v2.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 47f501302c0d 3.13.0-96-generic #143-Ubuntu SMP Mon Aug 29 20:15:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 5d5614f
          Default Java 1.8.0_111
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/17674/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17674/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17674/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 19s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. 0 mvndep 1m 41s Maven dependency ordering for branch +1 mvninstall 8m 27s trunk passed +1 compile 11m 25s trunk passed +1 checkstyle 1m 36s trunk passed +1 mvnsite 2m 44s trunk passed +1 mvneclipse 0m 57s trunk passed +1 findbugs 5m 2s trunk passed +1 javadoc 2m 11s trunk passed 0 mvndep 0m 18s Maven dependency ordering for patch +1 mvninstall 2m 7s the patch passed +1 compile 9m 42s the patch passed +1 javac 9m 42s the patch passed +1 checkstyle 1m 38s the patch passed +1 mvnsite 2m 56s the patch passed +1 mvneclipse 1m 3s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 5m 21s the patch passed +1 javadoc 2m 1s the patch passed +1 unit 9m 26s hadoop-common in the patch passed. +1 unit 1m 1s hadoop-hdfs-client in the patch passed. -1 unit 95m 56s hadoop-hdfs in the patch failed. +1 asflicense 0m 35s The patch does not generate ASF License warnings. 167m 53s Reason Tests Failed junit tests hadoop.hdfs.TestDFSStripedOutputStreamWithFailure120   hadoop.cli.TestErasureCodingCLI   hadoop.hdfs.TestRollingUpgrade Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11072 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12840651/HDFS-11072-v2.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 47f501302c0d 3.13.0-96-generic #143-Ubuntu SMP Mon Aug 29 20:15:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 5d5614f Default Java 1.8.0_111 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/17674/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17674/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17674/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          rakeshr Rakesh R added a comment - - edited

          Thanks SammiChen for the patch. Sometime back I had created HDFS-11082 to address the replicated EC policy case separately. Can we separate out these changes from this jira patch and push via HDFS-11082. I think, that would help to do more focused code reviews and get chance to add more unit test cases. Any thoughts? Also, please feel free to assign HDFS-11082 jira to you.

          Show
          rakeshr Rakesh R added a comment - - edited Thanks SammiChen for the patch. Sometime back I had created HDFS-11082 to address the replicated EC policy case separately. Can we separate out these changes from this jira patch and push via HDFS-11082 . I think, that would help to do more focused code reviews and get chance to add more unit test cases. Any thoughts? Also, please feel free to assign HDFS-11082 jira to you.
          Hide
          Sammi SammiChen added a comment -

          Hi Rakesh, sorry, I don't know you created HDFS-11082 for "replicated" policy, I originally throught you are going to fire a JIRA for

          It might also be nice to provide a CLI tool that prints the policies on the parent directories of a path.

          After some consideration, I think it's hard to implement the "unset" function without the "replicated" policy. So it's better to have the "replicated" policy implemented in this JIRA.
          And yes, I can take HDFS-11082 if that's OK.

          Show
          Sammi SammiChen added a comment - Hi Rakesh, sorry, I don't know you created HDFS-11082 for "replicated" policy, I originally throught you are going to fire a JIRA for It might also be nice to provide a CLI tool that prints the policies on the parent directories of a path. After some consideration, I think it's hard to implement the "unset" function without the "replicated" policy. So it's better to have the "replicated" policy implemented in this JIRA. And yes, I can take HDFS-11082 if that's OK.
          Hide
          Sammi SammiChen added a comment -

          1. fix style issues reported
          2. add erasurecode CLI test case
          3. add more Java test case
          4. double check failed test case
          5. Update document HDFSErasureCoding.md about how to use new "replicate" policy

          Show
          Sammi SammiChen added a comment - 1. fix style issues reported 2. add erasurecode CLI test case 3. add more Java test case 4. double check failed test case 5. Update document HDFSErasureCoding.md about how to use new "replicate" policy
          Hide
          rakeshr Rakesh R added a comment -

          Please go ahead. I will support in reviews.

          Show
          rakeshr Rakesh R added a comment - Please go ahead. I will support in reviews.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 11s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
          0 mvndep 1m 38s Maven dependency ordering for branch
          +1 mvninstall 7m 3s trunk passed
          +1 compile 9m 41s trunk passed
          +1 checkstyle 1m 34s trunk passed
          +1 mvnsite 2m 41s trunk passed
          +1 mvneclipse 0m 57s trunk passed
          +1 findbugs 4m 44s trunk passed
          +1 javadoc 1m 59s trunk passed
          0 mvndep 0m 15s Maven dependency ordering for patch
          +1 mvninstall 1m 54s the patch passed
          +1 compile 9m 18s the patch passed
          +1 javac 9m 18s the patch passed
          +1 checkstyle 1m 32s the patch passed
          +1 mvnsite 2m 38s the patch passed
          +1 mvneclipse 0m 57s the patch passed
          -1 whitespace 0m 0s The patch has 3 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 5m 7s the patch passed
          +1 javadoc 1m 58s the patch passed
          +1 unit 8m 8s hadoop-common in the patch passed.
          +1 unit 1m 3s hadoop-hdfs-client in the patch passed.
          -1 unit 62m 6s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 34s The patch does not generate ASF License warnings.
          127m 18s



          Reason Tests
          Failed junit tests hadoop.cli.TestErasureCodingCLI
            hadoop.hdfs.TestMaintenanceState



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11072
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12840825/HDFS-11072-v3.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux d0d33ba515f6 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 / 67d9f28
          Default Java 1.8.0_111
          findbugs v3.0.0
          whitespace https://builds.apache.org/job/PreCommit-HDFS-Build/17697/artifact/patchprocess/whitespace-eol.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/17697/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17697/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17697/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 11s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. 0 mvndep 1m 38s Maven dependency ordering for branch +1 mvninstall 7m 3s trunk passed +1 compile 9m 41s trunk passed +1 checkstyle 1m 34s trunk passed +1 mvnsite 2m 41s trunk passed +1 mvneclipse 0m 57s trunk passed +1 findbugs 4m 44s trunk passed +1 javadoc 1m 59s trunk passed 0 mvndep 0m 15s Maven dependency ordering for patch +1 mvninstall 1m 54s the patch passed +1 compile 9m 18s the patch passed +1 javac 9m 18s the patch passed +1 checkstyle 1m 32s the patch passed +1 mvnsite 2m 38s the patch passed +1 mvneclipse 0m 57s the patch passed -1 whitespace 0m 0s The patch has 3 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 5m 7s the patch passed +1 javadoc 1m 58s the patch passed +1 unit 8m 8s hadoop-common in the patch passed. +1 unit 1m 3s hadoop-hdfs-client in the patch passed. -1 unit 62m 6s hadoop-hdfs in the patch failed. +1 asflicense 0m 34s The patch does not generate ASF License warnings. 127m 18s Reason Tests Failed junit tests hadoop.cli.TestErasureCodingCLI   hadoop.hdfs.TestMaintenanceState Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11072 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12840825/HDFS-11072-v3.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux d0d33ba515f6 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 / 67d9f28 Default Java 1.8.0_111 findbugs v3.0.0 whitespace https://builds.apache.org/job/PreCommit-HDFS-Build/17697/artifact/patchprocess/whitespace-eol.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/17697/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17697/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17697/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          Sammi SammiChen added a comment -

          Thank you Rakesh R!

          Show
          Sammi SammiChen added a comment - Thank you Rakesh R !
          Hide
          Sammi SammiChen added a comment -

          1. take care whitespace
          2. Improve TestErasureCodingCLI script file testErasureCodingConf.xml

          Show
          Sammi SammiChen added a comment - 1. take care whitespace 2. Improve TestErasureCodingCLI script file testErasureCodingConf.xml
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 10s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
          0 mvndep 0m 16s Maven dependency ordering for branch
          +1 mvninstall 6m 46s trunk passed
          +1 compile 10m 2s trunk passed
          +1 checkstyle 1m 31s trunk passed
          +1 mvnsite 2m 45s trunk passed
          +1 mvneclipse 0m 55s trunk passed
          -1 findbugs 1m 51s hadoop-hdfs-project/hadoop-hdfs in trunk has 1 extant Findbugs warnings.
          +1 javadoc 1m 58s trunk passed
          0 mvndep 0m 15s Maven dependency ordering for patch
          +1 mvninstall 1m 51s the patch passed
          +1 compile 9m 19s the patch passed
          +1 javac 9m 19s the patch passed
          +1 checkstyle 1m 31s the patch passed
          +1 mvnsite 2m 38s the patch passed
          +1 mvneclipse 0m 51s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 5m 12s the patch passed
          +1 javadoc 1m 57s the patch passed
          +1 unit 8m 36s hadoop-common in the patch passed.
          +1 unit 1m 2s hadoop-hdfs-client in the patch passed.
          +1 unit 62m 27s hadoop-hdfs in the patch passed.
          +1 asflicense 0m 33s The patch does not generate ASF License warnings.
          126m 43s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11072
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841023/HDFS-11072-v4.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 9928754dfc9e 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 / 51e6c1c
          Default Java 1.8.0_111
          findbugs v3.0.0
          findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/17711/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17711/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17711/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 10s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. 0 mvndep 0m 16s Maven dependency ordering for branch +1 mvninstall 6m 46s trunk passed +1 compile 10m 2s trunk passed +1 checkstyle 1m 31s trunk passed +1 mvnsite 2m 45s trunk passed +1 mvneclipse 0m 55s trunk passed -1 findbugs 1m 51s hadoop-hdfs-project/hadoop-hdfs in trunk has 1 extant Findbugs warnings. +1 javadoc 1m 58s trunk passed 0 mvndep 0m 15s Maven dependency ordering for patch +1 mvninstall 1m 51s the patch passed +1 compile 9m 19s the patch passed +1 javac 9m 19s the patch passed +1 checkstyle 1m 31s the patch passed +1 mvnsite 2m 38s the patch passed +1 mvneclipse 0m 51s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 5m 12s the patch passed +1 javadoc 1m 57s the patch passed +1 unit 8m 36s hadoop-common in the patch passed. +1 unit 1m 2s hadoop-hdfs-client in the patch passed. +1 unit 62m 27s hadoop-hdfs in the patch passed. +1 asflicense 0m 33s The patch does not generate ASF License warnings. 126m 43s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11072 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841023/HDFS-11072-v4.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 9928754dfc9e 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 / 51e6c1c Default Java 1.8.0_111 findbugs v3.0.0 findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/17711/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/17711/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17711/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          Sammi SammiChen added a comment - - edited

          The Firebug warning is not relative to this patch. It is triggered by "org.apache.hadoop.hdfs.server.datanode.fsdataset.impl.BlockPoolSlice.validateIntegrityAndSetLength(File, long) " which has an update for HADOOP-10930. I will notify the owner of HADOOP-10930 for this issue.

          commit aeecfa24f4fb6af289920cbf8830c394e66bd78e
          Author: Xiaoyu Yao <xyao@apache.org>
          Date: Tue Nov 29 20:52:36 2016 -0800

          HADOOP-10930. Refactor: Wrap Datanode IO related operations. Contributed by
          Xiaoyu Yao.

          Show
          Sammi SammiChen added a comment - - edited The Firebug warning is not relative to this patch. It is triggered by "org.apache.hadoop.hdfs.server.datanode.fsdataset.impl.BlockPoolSlice.validateIntegrityAndSetLength(File, long) " which has an update for HADOOP-10930 . I will notify the owner of HADOOP-10930 for this issue. commit aeecfa24f4fb6af289920cbf8830c394e66bd78e Author: Xiaoyu Yao <xyao@apache.org> Date: Tue Nov 29 20:52:36 2016 -0800 HADOOP-10930 . Refactor: Wrap Datanode IO related operations. Contributed by Xiaoyu Yao.
          Hide
          rakeshr Rakesh R added a comment -

          Yes, we can ignore the findbug warning, its related to HDFS-10930, please see the discussion in that jira

          Show
          rakeshr Rakesh R added a comment - Yes, we can ignore the findbug warning, its related to HDFS-10930, please see the discussion in that jira
          Hide
          Sammi SammiChen added a comment -

          Hi Andrew Wang], I'm wondering if you have time help me reviewing the patch in your convenient time?

          Show
          Sammi SammiChen added a comment - Hi Andrew Wang ], I'm wondering if you have time help me reviewing the patch in your convenient time?
          Hide
          rakeshr Rakesh R added a comment - - edited

          Thanks SammiChen for the patch. I'm adding few thoughts, please take a look at it.

          1. Since this is not a regular ec policy which doesn't needs codec support, I'd prefer to use a value -1 or -128?. That way, in future, for the new EC policies can represent continuous numbers 4, 5 etc.
            public static final byte CONTINUOUS_REPLICATE_POLICY_ID = 4;
          2. What you think about exposing a new convenient API unsetErasureCodingPolicy(dir) and -unsetPolicy <path> command? Also, just a thought to make CONTINUOUS_REPLICATE_SCHEMA policy internal if the semantic of this is only for unset(going back to traditional block replication).
          3. In docs, could you please mention that one can change ec policy on a directory.
          Show
          rakeshr Rakesh R added a comment - - edited Thanks SammiChen for the patch. I'm adding few thoughts, please take a look at it. Since this is not a regular ec policy which doesn't needs codec support, I'd prefer to use a value -1 or -128?. That way, in future, for the new EC policies can represent continuous numbers 4, 5 etc. public static final byte CONTINUOUS_REPLICATE_POLICY_ID = 4; What you think about exposing a new convenient API unsetErasureCodingPolicy(dir) and -unsetPolicy <path> command? Also, just a thought to make CONTINUOUS_REPLICATE_SCHEMA policy internal if the semantic of this is only for unset(going back to traditional block replication). In docs, could you please mention that one can change ec policy on a directory.
          Hide
          andrew.wang Andrew Wang added a comment -

          Hi Sammi, thanks for working on this. Some review comments in addition to Rakesh's:

          • Can we just say "replication" rather than "continuous replicate"? e.g. "getReplicationPolicy" instead of "getContinuousReplicatePolicy"
          • Note that setting a "replication" EC policy is still different from unsetting. Unsetting means the policy will be inherited from an ancestor. Setting a "replication" policy means the "replication" policy will be used. Imagine a situation where there are "/a" has RS 6,3 set and "/a/b" has XOR 2,1 set. On "/a/b", unsetting vs. setting "replication" will have different effects. So we also need an unset API, similar to the unset storage policy API.

          Comment in ECPolicyManager, recommend reword like this:

            /*
             * This is a special policy. When this policy is applied to a directory, its
             * children will be replicated rather than inheriting an erasure coding policy
             * from an ancestor directory.
             *
             * This policy is only used when setting an erasure coding policy. It will not be
             * returned when get erasure coding policy is called.
             */
          
          • FSDirErasureCodingOp: rename "ecXAttrExisted" to "hasEcXAttr"
          • FSDirErasureCodingOp: should rename createErasureCodingPolicyXAttr to setErasureCodingPolicyXAttr, since it can now replace
          • Why do we hide the replication policy for calls to getErasureCodingPolicyForPath for directories? Makes sense for files since they are just replicated, but directory-level policies act like normal EC policies in that they can be inherited.
          • Rather than add new function getErasureCodingPolicyXAttrForLastINode to set a boolean, seems like we could call a "hasErasureCodingPolicy" method (the current one is also unused). Since this is only for paths that exist, it's safe to use FSDirectory.resolveLastINode instead of a for loop that skips nulls. We only need that for loop when creating a new path.
          • To assist with the above, I feel like we should have a getErasureCodingPolicy(INode) method that does this block in getErasureCodingPolicyForPath:
                  final XAttrFeature xaf = inode.getXAttrFeature();
                  if (xaf != null) {
                    XAttr xattr = xaf.getXAttr(XATTR_ERASURECODING_POLICY);
                    if (xattr != null) {
                      ByteArrayInputStream bIn = new ByteArrayInputStream(xattr.getValue());
                      DataInputStream dIn = new DataInputStream(bIn);
                      String ecPolicyName = WritableUtils.readString(dIn);
                      if (!ecPolicyName.equalsIgnoreCase(ErasureCodingPolicyManager
                          .getContinuousReplicatePolicy().getName())) {
                        return fsd.getFSNamesystem().getErasureCodingPolicyManager().
                            getPolicyByName(ecPolicyName);
                      } else {
                        return null;
                      }
                    }
                  }
          

          Documentation:

          • "Another purpose of this special policy is to unset the erasure coding policy of a directory back to the traditional replications.", I don't think we should say this, since we also support actually unsetting the EC policy. The replication policy is still a policy that overrides policies on ancestor directories.
          • Do the parameters "1-2-64K" have any meaning? If not, we should explain that they are meaningless, or hide the parameters so we don't need to talk about them.

          Tests:

          • It's better to use more specific asserts like assertNull, assertNotNull}, etc instead of just {{assertTrue
          • Would be good to create files with different replication factors.
          Show
          andrew.wang Andrew Wang added a comment - Hi Sammi, thanks for working on this. Some review comments in addition to Rakesh's: Can we just say "replication" rather than "continuous replicate"? e.g. "getReplicationPolicy" instead of "getContinuousReplicatePolicy" Note that setting a "replication" EC policy is still different from unsetting. Unsetting means the policy will be inherited from an ancestor. Setting a "replication" policy means the "replication" policy will be used. Imagine a situation where there are "/a" has RS 6,3 set and "/a/b" has XOR 2,1 set. On "/a/b", unsetting vs. setting "replication" will have different effects. So we also need an unset API, similar to the unset storage policy API. Comment in ECPolicyManager, recommend reword like this: /* * This is a special policy. When this policy is applied to a directory, its * children will be replicated rather than inheriting an erasure coding policy * from an ancestor directory. * * This policy is only used when setting an erasure coding policy. It will not be * returned when get erasure coding policy is called. */ FSDirErasureCodingOp: rename "ecXAttrExisted" to "hasEcXAttr" FSDirErasureCodingOp: should rename createErasureCodingPolicyXAttr to setErasureCodingPolicyXAttr, since it can now replace Why do we hide the replication policy for calls to getErasureCodingPolicyForPath for directories? Makes sense for files since they are just replicated, but directory-level policies act like normal EC policies in that they can be inherited. Rather than add new function getErasureCodingPolicyXAttrForLastINode to set a boolean, seems like we could call a "hasErasureCodingPolicy" method (the current one is also unused). Since this is only for paths that exist, it's safe to use FSDirectory.resolveLastINode instead of a for loop that skips nulls. We only need that for loop when creating a new path. To assist with the above, I feel like we should have a getErasureCodingPolicy(INode) method that does this block in getErasureCodingPolicyForPath: final XAttrFeature xaf = inode.getXAttrFeature(); if (xaf != null ) { XAttr xattr = xaf.getXAttr(XATTR_ERASURECODING_POLICY); if (xattr != null ) { ByteArrayInputStream bIn = new ByteArrayInputStream(xattr.getValue()); DataInputStream dIn = new DataInputStream(bIn); String ecPolicyName = WritableUtils.readString(dIn); if (!ecPolicyName.equalsIgnoreCase(ErasureCodingPolicyManager .getContinuousReplicatePolicy().getName())) { return fsd.getFSNamesystem().getErasureCodingPolicyManager(). getPolicyByName(ecPolicyName); } else { return null ; } } } Documentation: "Another purpose of this special policy is to unset the erasure coding policy of a directory back to the traditional replications.", I don't think we should say this, since we also support actually unsetting the EC policy. The replication policy is still a policy that overrides policies on ancestor directories. Do the parameters "1-2-64K" have any meaning? If not, we should explain that they are meaningless, or hide the parameters so we don't need to talk about them. Tests: It's better to use more specific asserts like assertNull , assertNotNull}, etc instead of just {{assertTrue Would be good to create files with different replication factors.
          Hide
          Sammi SammiChen added a comment -

          Thank you Rakesh so much for reviewing the patch! And very good advice.

          1. For replication policy ID, I will prefer to use 128 since there are a lot of checkers checking that policy ID cannot be negative number. I guess we will not have as many as 128 polices in future.
          2. a new API unsetErasureCodingPolicy and a new sub command are very good suggestions. Will follow it in the new patch.
          3. Sure, I will improve the docs.

          Show
          Sammi SammiChen added a comment - Thank you Rakesh so much for reviewing the patch! And very good advice. 1. For replication policy ID, I will prefer to use 128 since there are a lot of checkers checking that policy ID cannot be negative number. I guess we will not have as many as 128 polices in future. 2. a new API unsetErasureCodingPolicy and a new sub command are very good suggestions. Will follow it in the new patch. 3. Sure, I will improve the docs.
          Hide
          Sammi SammiChen added a comment - - edited

          Andrew, thanks very much for taking time review the patch!

          Can we just say "replication" rather than "continuous replicate"? e.g. "getReplicationPolicy" instead of "getContinuousReplicatePolicy"

          "continuous replicate" is chosen because I thought there is the combination of "replication" plus "erasure coding", the planed phase 2 of erasure coding. So I'm use "continuous replicate" to distinguish future "erasure coding replicate". Does it make sense?

          Note that setting a "replication" EC policy is still different from unsetting. Unsetting means the policy will be inherited from an ancestor. Setting a "replication" policy means the "replication" policy will be used. Imagine a situation where there are "/a" has RS 6,3 set and "/a/b" has XOR 2,1 set. On "/a/b", unsetting vs. setting "replication" will have different effects. So we also need an unset API, similar to the unset storage policy API.

          I agree with you and the implementation matches your thoughts. And I will add a new unset API.

          Do the parameters "1-2-64K" have any meaning? If not, we should explain that they are meaningless, or hide the parameters so we don't need to talk about them.

          "1-2-64K" is auto generated from the schema when replicate policy is defined. The data is meaningless. At the first, I use the "null" as schema to define the policy, then I found there is checker about schema can't be null. And then I use schema (0-0-0). It breaks other checkers. I think we would like to keep these checkers to avoid mistakes made by real ec policy, so at the end, I choose "1-2-64k", which means 1 data block, 2 parity blocks, kind of matching the default 3 replication case. As Rakesh has suggested to add a new unset API and a new unset policy sub command in "erasurecode", makes the replicate policy internal. So user will not see the policy unless they read the source code.

          I will take care of all other comments in the new patch.

          Show
          Sammi SammiChen added a comment - - edited Andrew, thanks very much for taking time review the patch! Can we just say "replication" rather than "continuous replicate"? e.g. "getReplicationPolicy" instead of "getContinuousReplicatePolicy" "continuous replicate" is chosen because I thought there is the combination of "replication" plus "erasure coding", the planed phase 2 of erasure coding. So I'm use "continuous replicate" to distinguish future "erasure coding replicate". Does it make sense? Note that setting a "replication" EC policy is still different from unsetting. Unsetting means the policy will be inherited from an ancestor. Setting a "replication" policy means the "replication" policy will be used. Imagine a situation where there are "/a" has RS 6,3 set and "/a/b" has XOR 2,1 set. On "/a/b", unsetting vs. setting "replication" will have different effects. So we also need an unset API, similar to the unset storage policy API. I agree with you and the implementation matches your thoughts. And I will add a new unset API. Do the parameters "1-2-64K" have any meaning? If not, we should explain that they are meaningless, or hide the parameters so we don't need to talk about them. "1-2-64K" is auto generated from the schema when replicate policy is defined. The data is meaningless. At the first, I use the "null" as schema to define the policy, then I found there is checker about schema can't be null. And then I use schema (0-0-0). It breaks other checkers. I think we would like to keep these checkers to avoid mistakes made by real ec policy, so at the end, I choose "1-2-64k", which means 1 data block, 2 parity blocks, kind of matching the default 3 replication case. As Rakesh has suggested to add a new unset API and a new unset policy sub command in "erasurecode", makes the replicate policy internal. So user will not see the policy unless they read the source code. I will take care of all other comments in the new patch.
          Hide
          Sammi SammiChen added a comment -

          I'm rethinking about Rakesh and Andrew's comments. A directory can be one of these three states,
          1. it has normal ec policy on itself
          2. it has replication policy on itself
          3 it has nothing policy on itself. it inherits whatever from its parent group

          From user's point of view, the new created directory has state 3. If user wants the directory stop inheriting, he/she can use setErasureCodingPolicy API to set either normal ec policy or replication policy on directory, then the directory transforms to state 1 or state 2 respectively. To transfer directory from state 1 or state 2 back to state 3, we need a removeErasureCodingPolicy API which will remove both the normal ec and replication policy.

          Given the above 3 states, when user call getErasureCodingPolicy, the API will return the normal ec policy. The replication policy will not be returned, both directory and file. I think user only cares about whether the directory is a ec directory or a 3 way replication directory. I mean if a directory in state 2, user don't care about whether it's inherited or because of there is replication policy on itself. Andrew, can you explain why you think for a directory, the replication policy should be return when getErasureCodingPolicy is called?

          So it seems keep the replication policy internal and hide from end user is not feasible. We requires user explicitly set the replication policy through setErasureCodingPolicy API. Unless we introduce another policy manipulation API, such as "setDefaultReplicationPolicy" which handles change directory from ec policy to replication policy.

          What do you think? Andrew Wang , Rakesh R

          Show
          Sammi SammiChen added a comment - I'm rethinking about Rakesh and Andrew's comments. A directory can be one of these three states, 1. it has normal ec policy on itself 2. it has replication policy on itself 3 it has nothing policy on itself. it inherits whatever from its parent group From user's point of view, the new created directory has state 3. If user wants the directory stop inheriting, he/she can use setErasureCodingPolicy API to set either normal ec policy or replication policy on directory, then the directory transforms to state 1 or state 2 respectively. To transfer directory from state 1 or state 2 back to state 3, we need a removeErasureCodingPolicy API which will remove both the normal ec and replication policy. Given the above 3 states, when user call getErasureCodingPolicy, the API will return the normal ec policy. The replication policy will not be returned, both directory and file. I think user only cares about whether the directory is a ec directory or a 3 way replication directory. I mean if a directory in state 2, user don't care about whether it's inherited or because of there is replication policy on itself. Andrew, can you explain why you think for a directory, the replication policy should be return when getErasureCodingPolicy is called? So it seems keep the replication policy internal and hide from end user is not feasible. We requires user explicitly set the replication policy through setErasureCodingPolicy API. Unless we introduce another policy manipulation API, such as "setDefaultReplicationPolicy" which handles change directory from ec policy to replication policy. What do you think? Andrew Wang , Rakesh R
          Hide
          andrew.wang Andrew Wang added a comment -

          Thanks for the detailed writeup Sammi!

          Andrew, can you explain why you think for a directory, the replication policy should be return when getErasureCodingPolicy is called?

          I was thinking of a usecase where a user wants to redo the policies on an directory tree. Unless they can distinguish between states 1 and 2 vs 3 via a get API, they need to call set/remove on every directory to get exactly what they want. Another usecase is distcp, where you might want to exactly replicate the same storage policy setup on a destination cluster.

          Looking at StoragePolicy though, it just returns the inherited policy. I don't see a way to check if a policy is inherited or explicitly set. IMO this is a flaw (particularly for distcp), but it's better to follow suit for continuity. It's also less bad for EC since there's no way to change the EC policy for a file.

          Also referencing StoragePolicy, there's the idea of a default storage policy for the cluster. This is hardcoded to HOT, and is returned when you call getStoragePolicy. To align with getStoragePolicy, arguably getECPolicy should return a special "replicated" ECPolicy, but that makes isErasureCoded checks more complicated.

          So, all said, let's just return the inherited EC policy if it's not replicated. We also need to validate the isErasureCoded checks internally, since I know for instance we restrict the set of storage policies that work on erasure coded files.

          Unless we introduce another policy manipulation API, such as "setDefaultReplicationPolicy" which handles change directory from ec policy to replication policy.

          I like this idea, since calling the replication policy an "ECPolicy" is a misnomer. It's also confusing if we make people set it via setECPolicy but don't return it in getECPolicy.

          Show
          andrew.wang Andrew Wang added a comment - Thanks for the detailed writeup Sammi! Andrew, can you explain why you think for a directory, the replication policy should be return when getErasureCodingPolicy is called? I was thinking of a usecase where a user wants to redo the policies on an directory tree. Unless they can distinguish between states 1 and 2 vs 3 via a get API, they need to call set/remove on every directory to get exactly what they want. Another usecase is distcp, where you might want to exactly replicate the same storage policy setup on a destination cluster. Looking at StoragePolicy though, it just returns the inherited policy. I don't see a way to check if a policy is inherited or explicitly set. IMO this is a flaw (particularly for distcp), but it's better to follow suit for continuity. It's also less bad for EC since there's no way to change the EC policy for a file. Also referencing StoragePolicy, there's the idea of a default storage policy for the cluster. This is hardcoded to HOT, and is returned when you call getStoragePolicy. To align with getStoragePolicy, arguably getECPolicy should return a special "replicated" ECPolicy, but that makes isErasureCoded checks more complicated. So, all said, let's just return the inherited EC policy if it's not replicated. We also need to validate the isErasureCoded checks internally, since I know for instance we restrict the set of storage policies that work on erasure coded files. Unless we introduce another policy manipulation API, such as "setDefaultReplicationPolicy" which handles change directory from ec policy to replication policy. I like this idea, since calling the replication policy an "ECPolicy" is a misnomer. It's also confusing if we make people set it via setECPolicy but don't return it in getECPolicy.
          Hide
          Sammi SammiChen added a comment - - edited

          Hi Andrew, thanks for sharing your thoughts. Talking about redo policy on an directory tree, even we provide user knowledge about whether the policy is inherited or not, user still need to go through the tree to undo the policy one by one. Because the sub directory can have its own policy by overriding parent directory's policy. Unless we have feature like "replace all child with this directory's policy" which is not feasible in distributed environment. For distcp, how about add a option to explicitly reserve inherited policy(erasure coding policy or storage policy). Just a thought, I'm not sure if this will introduce massive complexity into distcp's implementation.

          I'm glad you also like the idea to introduce a new API. So, for erasure coding policy, there will be 4 API.
          1. setErasureCodingPolicy
          set ec policy on directory
          2. removeErasureCodingPolicy
          remove policy(ec or replication) on directory, after removal, directory will back to inheriting from parent directory (word "remove" is used more often in DistributedFileSystem API name than unset)
          3. setDefaultReplicationPolicy
          set replication on directory. This is only useful when user wants the directory from stop inheriting from it's parent's ec policy.
          4. getErasureCodingPolicy
          return the policy set by setErasureCodingPolicy

          But even introduce a new API to handle replication case, it's still kind of complicated. The complexity is introduced by the "replication" policy. From my limited knowledge, ec is suggested for cold data, and replication is suggested for hot data. Set replication on a sub directory under a parent ec directory is useful in cases that the cold data back to hot again, right? But I don't know how often is this scenario, and is it worthy to introduce the complexity to handle the case. So another idea in my mind is we can introduce "replication" policy later until we know it's very useful to end user.

          Anyway, I'm OK with the 4 API solution. Just want to make sure we are at the same page before I start to refine the patch.

          Show
          Sammi SammiChen added a comment - - edited Hi Andrew, thanks for sharing your thoughts. Talking about redo policy on an directory tree, even we provide user knowledge about whether the policy is inherited or not, user still need to go through the tree to undo the policy one by one. Because the sub directory can have its own policy by overriding parent directory's policy. Unless we have feature like "replace all child with this directory's policy" which is not feasible in distributed environment. For distcp, how about add a option to explicitly reserve inherited policy(erasure coding policy or storage policy). Just a thought, I'm not sure if this will introduce massive complexity into distcp's implementation. I'm glad you also like the idea to introduce a new API. So, for erasure coding policy, there will be 4 API. 1. setErasureCodingPolicy set ec policy on directory 2. removeErasureCodingPolicy remove policy(ec or replication) on directory, after removal, directory will back to inheriting from parent directory (word "remove" is used more often in DistributedFileSystem API name than unset) 3. setDefaultReplicationPolicy set replication on directory. This is only useful when user wants the directory from stop inheriting from it's parent's ec policy. 4. getErasureCodingPolicy return the policy set by setErasureCodingPolicy But even introduce a new API to handle replication case, it's still kind of complicated. The complexity is introduced by the "replication" policy. From my limited knowledge, ec is suggested for cold data, and replication is suggested for hot data. Set replication on a sub directory under a parent ec directory is useful in cases that the cold data back to hot again, right? But I don't know how often is this scenario, and is it worthy to introduce the complexity to handle the case. So another idea in my mind is we can introduce "replication" policy later until we know it's very useful to end user. Anyway, I'm OK with the 4 API solution. Just want to make sure we are at the same page before I start to refine the patch.
          Hide
          rakeshr Rakesh R added a comment -

          Thanks SammiChen for the analysis.

          2. removeErasureCodingPolicy

          Since we are already using set/unset API pattern for the storage policy, I'd prefer to create #unsetErasureCodingPolicy() API instead of #removeErasureCodingPolicy() API.

          3. setDefaultReplicationPolicy
          set replication on directory. This is only useful when user wants the directory from stop inheriting from it's parent's ec policy.

          I understand there will be many use cases, like distcp(Thanks Andrew Wang for pointing out this) etc. IMHO, implementing this logic requires more focused coding/unit testing and reviews. How about separating out this task alone and pushing via HDFS-11082 jira. Please feel free to assign HDFS-11082 task to you.

          Show
          rakeshr Rakesh R added a comment - Thanks SammiChen for the analysis. 2. removeErasureCodingPolicy Since we are already using set/unset API pattern for the storage policy, I'd prefer to create #unsetErasureCodingPolicy() API instead of #removeErasureCodingPolicy() API. 3. setDefaultReplicationPolicy set replication on directory. This is only useful when user wants the directory from stop inheriting from it's parent's ec policy. I understand there will be many use cases, like distcp(Thanks Andrew Wang for pointing out this) etc. IMHO, implementing this logic requires more focused coding/unit testing and reviews. How about separating out this task alone and pushing via HDFS-11082 jira. Please feel free to assign HDFS-11082 task to you.
          Hide
          Sammi SammiChen added a comment -

          I totally agree with you Rakesh R! The replication policy is absolutely useful when user wants to stop inheriting a directory from its parent's ec policy. But in the meantime, it also introduces the concept complexity to user. So maybe it's better we do this part later in JIRA HDFS-11082. Thanks Rakesh, I will update the patch accordingly!

          Show
          Sammi SammiChen added a comment - I totally agree with you Rakesh R ! The replication policy is absolutely useful when user wants to stop inheriting a directory from its parent's ec policy. But in the meantime, it also introduces the concept complexity to user. So maybe it's better we do this part later in JIRA HDFS-11082 . Thanks Rakesh, I will update the patch accordingly!
          Hide
          Sammi SammiChen added a comment -

          1. add unsetErasureCodingPolicy API
          2. add "-unsetPolicy" sub-command to "hdfs erasurecode" command
          3. "replication" policy will be introduced with HDFS-11082 JIRA
          4. Refine code based on comments from Rakesh and Andrew

          Show
          Sammi SammiChen added a comment - 1. add unsetErasureCodingPolicy API 2. add "-unsetPolicy" sub-command to "hdfs erasurecode" command 3. "replication" policy will be introduced with HDFS-11082 JIRA 4. Refine code based on comments from Rakesh and Andrew
          Hide
          andrew.wang Andrew Wang added a comment -

          Thanks for the rev Sammi, looks like we're getting close. Agree that we can focus on getting this in, and worry about the replication policy later.

          Some code review comments:

          • nit: can combine these two lines into one in FSDirErasureCodingOp:
              FSPermissionChecker pc = null;
              pc = fsn.getPermissionChecker();
          
          • removeErasureCodingPolicyXAttr can be private
          • IOException text says "Attempt to unset an erasure coding policy from a file", prefer if we be more explicit about the error and say "Cannot unset the erasure coding policy on a file".
          • Why do we need the new getLastCompleteINode method? IIUC an IIP has nulls if that path component doesn't exist, but that only happens when we're creating a new inode. Maybe we should test calling these APIs on a path that does not exist.
          • In TestErasureCodingPolicies, should say "policies are supported" rather than "policies is supported" in both places
          • Additional unit test ideas: setting and unsetting on a file, unsetting when not set, setting twice on the same directory with different policies
          Show
          andrew.wang Andrew Wang added a comment - Thanks for the rev Sammi, looks like we're getting close. Agree that we can focus on getting this in, and worry about the replication policy later. Some code review comments: nit: can combine these two lines into one in FSDirErasureCodingOp: FSPermissionChecker pc = null ; pc = fsn.getPermissionChecker(); removeErasureCodingPolicyXAttr can be private IOException text says "Attempt to unset an erasure coding policy from a file", prefer if we be more explicit about the error and say "Cannot unset the erasure coding policy on a file". Why do we need the new getLastCompleteINode method? IIUC an IIP has nulls if that path component doesn't exist, but that only happens when we're creating a new inode. Maybe we should test calling these APIs on a path that does not exist. In TestErasureCodingPolicies, should say "policies are supported" rather than "policies is supported" in both places Additional unit test ideas: setting and unsetting on a file, unsetting when not set, setting twice on the same directory with different policies
          Hide
          drankye Kai Zheng added a comment -

          Hi Andrew Wang, this hasn't built for some days due to some unknown Jenkins problem. I manually triggered the building by input of the jira number, but the task failed. Could you help with a check and give some hint? This issue doesn't look some special. Thank you.

          Show
          drankye Kai Zheng added a comment - Hi Andrew Wang , this hasn't built for some days due to some unknown Jenkins problem. I manually triggered the building by input of the jira number, but the task failed. Could you help with a check and give some hint? This issue doesn't look some special. Thank you.
          Hide
          andrew.wang Andrew Wang added a comment -

          Looking at some other stalled precommits, it looks like the H9 build box is bad. I filed INFRA-13234, also ping Allen Wittenauer in case he has ideas.

          Show
          andrew.wang Andrew Wang added a comment - Looking at some other stalled precommits, it looks like the H9 build box is bad. I filed INFRA-13234 , also ping Allen Wittenauer in case he has ideas.
          Hide
          aw Allen Wittenauer added a comment - - edited

          Wrote a job that applied a "scorched earth" policy to H9's docker situation. Looks like mesos' reviewbot is now properly building docker images again, so I suspect everything that is using yetus will work too.

          Show
          aw Allen Wittenauer added a comment - - edited Wrote a job that applied a "scorched earth" policy to H9's docker situation. Looks like mesos' reviewbot is now properly building docker images again, so I suspect everything that is using yetus will work too.
          Hide
          andrew.wang Andrew Wang added a comment -

          Thanks Allen for the quick turnaround! I retriggered precommit for this JIRA.

          Show
          andrew.wang Andrew Wang added a comment - Thanks Allen for the quick turnaround! I retriggered precommit for this JIRA.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 20m 52s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
          0 mvndep 0m 39s Maven dependency ordering for branch
          +1 mvninstall 14m 43s trunk passed
          +1 compile 1m 23s trunk passed
          +1 checkstyle 0m 38s trunk passed
          +1 mvnsite 1m 26s trunk passed
          +1 mvneclipse 0m 30s trunk passed
          +1 findbugs 3m 11s trunk passed
          +1 javadoc 1m 5s trunk passed
          0 mvndep 0m 8s Maven dependency ordering for patch
          +1 mvninstall 1m 22s the patch passed
          +1 compile 1m 25s the patch passed
          +1 cc 1m 25s the patch passed
          +1 javac 1m 25s the patch passed
          -0 checkstyle 0m 35s hadoop-hdfs-project: The patch generated 2 new + 433 unchanged - 0 fixed = 435 total (was 433)
          +1 mvnsite 1m 23s the patch passed
          +1 mvneclipse 0m 21s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 3m 34s the patch passed
          +1 javadoc 0m 56s the patch passed
          +1 unit 0m 56s hadoop-hdfs-client in the patch passed.
          -1 unit 94m 1s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 29s The patch does not generate ASF License warnings.
          151m 37s



          Reason Tests
          Failed junit tests hadoop.tracing.TestTracing
            hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure
            hadoop.hdfs.server.namenode.TestReconstructStripedBlocks



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11072
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12844893/HDFS-11072-v5.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle cc xml
          uname Linux 0af8569b4e09 3.13.0-96-generic #143-Ubuntu SMP Mon Aug 29 20:15:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 0a55bd8
          Default Java 1.8.0_111
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18038/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18038/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18038/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18038/console
          Powered by Apache Yetus 0.5.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 20m 52s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. 0 mvndep 0m 39s Maven dependency ordering for branch +1 mvninstall 14m 43s trunk passed +1 compile 1m 23s trunk passed +1 checkstyle 0m 38s trunk passed +1 mvnsite 1m 26s trunk passed +1 mvneclipse 0m 30s trunk passed +1 findbugs 3m 11s trunk passed +1 javadoc 1m 5s trunk passed 0 mvndep 0m 8s Maven dependency ordering for patch +1 mvninstall 1m 22s the patch passed +1 compile 1m 25s the patch passed +1 cc 1m 25s the patch passed +1 javac 1m 25s the patch passed -0 checkstyle 0m 35s hadoop-hdfs-project: The patch generated 2 new + 433 unchanged - 0 fixed = 435 total (was 433) +1 mvnsite 1m 23s the patch passed +1 mvneclipse 0m 21s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 3m 34s the patch passed +1 javadoc 0m 56s the patch passed +1 unit 0m 56s hadoop-hdfs-client in the patch passed. -1 unit 94m 1s hadoop-hdfs in the patch failed. +1 asflicense 0m 29s The patch does not generate ASF License warnings. 151m 37s Reason Tests Failed junit tests hadoop.tracing.TestTracing   hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure   hadoop.hdfs.server.namenode.TestReconstructStripedBlocks Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11072 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12844893/HDFS-11072-v5.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle cc xml uname Linux 0af8569b4e09 3.13.0-96-generic #143-Ubuntu SMP Mon Aug 29 20:15:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 0a55bd8 Default Java 1.8.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18038/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/18038/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18038/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18038/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          Sammi SammiChen added a comment -

          Thanks Andrew Wang for the comments. I will upload a new patch to address all the concerns.

          Show
          Sammi SammiChen added a comment - Thanks Andrew Wang for the comments. I will upload a new patch to address all the concerns.
          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 3 new or modified test files.
          0 mvndep 0m 30s Maven dependency ordering for branch
          +1 mvninstall 13m 41s trunk passed
          +1 compile 1m 31s trunk passed
          +1 checkstyle 0m 39s trunk passed
          +1 mvnsite 1m 32s trunk passed
          +1 mvneclipse 0m 27s trunk passed
          +1 findbugs 3m 36s trunk passed
          +1 javadoc 1m 6s trunk passed
          0 mvndep 0m 7s Maven dependency ordering for patch
          +1 mvninstall 1m 34s the patch passed
          +1 compile 1m 40s the patch passed
          +1 cc 1m 40s the patch passed
          +1 javac 1m 40s the patch passed
          +1 checkstyle 0m 35s the patch passed
          +1 mvnsite 1m 36s the patch passed
          +1 mvneclipse 0m 24s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 3m 33s the patch passed
          +1 javadoc 0m 57s the patch passed
          +1 unit 0m 57s hadoop-hdfs-client in the patch passed.
          -1 unit 91m 39s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          128m 8s



          Reason Tests
          Failed junit tests hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure
            hadoop.hdfs.TestFileCorruption



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11072
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12846258/HDFS-11072-v6.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle cc xml
          uname Linux fa33ff6dc558 3.13.0-96-generic #143-Ubuntu SMP Mon Aug 29 20:15:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / f59e36b
          Default Java 1.8.0_111
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18105/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18105/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18105/console
          Powered by Apache Yetus 0.5.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 3 new or modified test files. 0 mvndep 0m 30s Maven dependency ordering for branch +1 mvninstall 13m 41s trunk passed +1 compile 1m 31s trunk passed +1 checkstyle 0m 39s trunk passed +1 mvnsite 1m 32s trunk passed +1 mvneclipse 0m 27s trunk passed +1 findbugs 3m 36s trunk passed +1 javadoc 1m 6s trunk passed 0 mvndep 0m 7s Maven dependency ordering for patch +1 mvninstall 1m 34s the patch passed +1 compile 1m 40s the patch passed +1 cc 1m 40s the patch passed +1 javac 1m 40s the patch passed +1 checkstyle 0m 35s the patch passed +1 mvnsite 1m 36s the patch passed +1 mvneclipse 0m 24s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 3m 33s the patch passed +1 javadoc 0m 57s the patch passed +1 unit 0m 57s hadoop-hdfs-client in the patch passed. -1 unit 91m 39s hadoop-hdfs in the patch failed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 128m 8s Reason Tests Failed junit tests hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure   hadoop.hdfs.TestFileCorruption Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11072 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12846258/HDFS-11072-v6.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle cc xml uname Linux fa33ff6dc558 3.13.0-96-generic #143-Ubuntu SMP Mon Aug 29 20:15:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / f59e36b Default Java 1.8.0_111 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/18105/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18105/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18105/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          andrew.wang Andrew Wang added a comment -

          I'm almost +1, thanks for the quick rev Sammi,

          • In the new shell command, we get the EC policy before unsetting to check if there is a policy already set. unsetStoragePolicy and the Java unsetECPolicy API don't seem to error in this case, so I think we should just call unset without checking. This also requires a fix in the user docs. As a general comment, I prefer to surface errors on the NN rather than in the client code, for consistency between the Java and shell APIs.
          • Nit: "unexist" -> "non-existent" in the comments in testNonExistentDir
          Show
          andrew.wang Andrew Wang added a comment - I'm almost +1, thanks for the quick rev Sammi, In the new shell command, we get the EC policy before unsetting to check if there is a policy already set. unsetStoragePolicy and the Java unsetECPolicy API don't seem to error in this case, so I think we should just call unset without checking. This also requires a fix in the user docs. As a general comment, I prefer to surface errors on the NN rather than in the client code, for consistency between the Java and shell APIs. Nit: "unexist" -> "non-existent" in the comments in testNonExistentDir
          Hide
          Sammi SammiChen added a comment - - edited

          Thanks Andrew Wang ! I agree with the general comment. Will upload a new patch to address point 1&2.

          Show
          Sammi SammiChen added a comment - - edited Thanks Andrew Wang ! I agree with the general comment. Will upload a new patch to address point 1&2.
          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 3 new or modified test files.
          0 mvndep 0m 26s Maven dependency ordering for branch
          +1 mvninstall 13m 19s trunk passed
          +1 compile 1m 30s trunk passed
          +1 checkstyle 0m 36s trunk passed
          +1 mvnsite 1m 30s trunk passed
          +1 mvneclipse 0m 27s trunk passed
          +1 findbugs 3m 21s trunk passed
          +1 javadoc 1m 18s trunk passed
          0 mvndep 0m 10s Maven dependency ordering for patch
          +1 mvninstall 1m 47s the patch passed
          +1 compile 1m 45s the patch passed
          +1 cc 1m 45s the patch passed
          +1 javac 1m 45s the patch passed
          +1 checkstyle 0m 39s the patch passed
          +1 mvnsite 1m 40s the patch passed
          +1 mvneclipse 0m 26s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 2s The patch has no ill-formed XML file.
          +1 findbugs 4m 36s the patch passed
          +1 javadoc 1m 13s the patch passed
          +1 unit 1m 22s hadoop-hdfs-client in the patch passed.
          -1 unit 75m 18s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 20s The patch does not generate ASF License warnings.
          113m 57s



          Reason Tests
          Failed junit tests hadoop.hdfs.server.datanode.fsdataset.impl.TestLazyPersistReplicaRecovery



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11072
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12846498/HDFS-11072-v7.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle cc xml
          uname Linux 71caff72c614 3.13.0-103-generic #150-Ubuntu SMP Thu Nov 24 10:34:17 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 945db55
          Default Java 1.8.0_111
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18123/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18123/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18123/console
          Powered by Apache Yetus 0.5.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 3 new or modified test files. 0 mvndep 0m 26s Maven dependency ordering for branch +1 mvninstall 13m 19s trunk passed +1 compile 1m 30s trunk passed +1 checkstyle 0m 36s trunk passed +1 mvnsite 1m 30s trunk passed +1 mvneclipse 0m 27s trunk passed +1 findbugs 3m 21s trunk passed +1 javadoc 1m 18s trunk passed 0 mvndep 0m 10s Maven dependency ordering for patch +1 mvninstall 1m 47s the patch passed +1 compile 1m 45s the patch passed +1 cc 1m 45s the patch passed +1 javac 1m 45s the patch passed +1 checkstyle 0m 39s the patch passed +1 mvnsite 1m 40s the patch passed +1 mvneclipse 0m 26s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 2s The patch has no ill-formed XML file. +1 findbugs 4m 36s the patch passed +1 javadoc 1m 13s the patch passed +1 unit 1m 22s hadoop-hdfs-client in the patch passed. -1 unit 75m 18s hadoop-hdfs in the patch failed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 113m 57s Reason Tests Failed junit tests hadoop.hdfs.server.datanode.fsdataset.impl.TestLazyPersistReplicaRecovery Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11072 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12846498/HDFS-11072-v7.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle cc xml uname Linux 71caff72c614 3.13.0-103-generic #150-Ubuntu SMP Thu Nov 24 10:34:17 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 945db55 Default Java 1.8.0_111 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/18123/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18123/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18123/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          andrew.wang Andrew Wang added a comment -

          Looks great, thanks Sammi! I re-ran the failed test locally, looks like a flake. Committed to trunk.

          Show
          andrew.wang Andrew Wang added a comment - Looks great, thanks Sammi! I re-ran the failed test locally, looks like a flake. Committed to trunk.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11102 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11102/)
          HDFS-11072. Add ability to unset and change directory EC policy. (wang: rev e69231658dc4a79da936e6856017b5c4f6124ecb)

          • (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/ClientNamenodeProtocolTranslatorPB.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/ClientNamenodeProtocolServerSideTranslatorPB.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/proto/ClientNamenodeProtocol.proto
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/erasurecode/ECCommand.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/proto/erasurecoding.proto
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestErasureCodingPolicies.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/resources/testErasureCodingConf.xml
          • (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/site/markdown/HDFSErasureCoding.md
          • (add) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestUnsetAndChangeDirectoryEcPolicy.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/client/HdfsAdmin.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirErasureCodingOp.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11102 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11102/ ) HDFS-11072 . Add ability to unset and change directory EC policy. (wang: rev e69231658dc4a79da936e6856017b5c4f6124ecb) (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocolPB/ClientNamenodeProtocolTranslatorPB.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/ClientNamenodeProtocolServerSideTranslatorPB.java (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/proto/ClientNamenodeProtocol.proto (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/erasurecode/ECCommand.java (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/proto/erasurecoding.proto (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestErasureCodingPolicies.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/resources/testErasureCodingConf.xml (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/site/markdown/HDFSErasureCoding.md (add) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestUnsetAndChangeDirectoryEcPolicy.java (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/client/HdfsAdmin.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirErasureCodingOp.java
          Hide
          Sammi SammiChen added a comment -

          Thanks Andrew, for all the reviewing efforts and commit!

          Show
          Sammi SammiChen added a comment - Thanks Andrew, for all the reviewing efforts and commit!

            People

            • Assignee:
              Sammi SammiChen
              Reporter:
              andrew.wang Andrew Wang
            • Votes:
              0 Vote for this issue
              Watchers:
              14 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development