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

Refactor EC CLI to be similar to storage policies CLI

    Details

    • Target Version/s:
    • Hadoop Flags:
      Incompatible change
    • Release Note:
      The `hdfs ec` CLI command has been substantially reworked to make the calling patterns more similar to the `hdfs storagepolicies` command. See `hdfs ec -help` and the HDFS erasure coding documentation for more information.

      Description

      The hdfs erasurecode CLI is similar to hdfs storagepolicies in terms of functionality, but different in terms of behavior. Let's refactor ECCli to be more similar to the various Admin classes we already have, and also make its calling syntax mimic hdfs storagepolicies as closely as possible.

      1. HDFS-11426.001.patch
        36 kB
        Andrew Wang
      2. HDFS-11426.002.patch
        46 kB
        Andrew Wang
      3. HDFS-11426.003.patch
        48 kB
        Andrew Wang

        Issue Links

          Activity

          Hide
          andrew.wang Andrew Wang added a comment -

          Patch attached, I borrowed heavily from StoragePolicyAdmin. I think the SP APIs isn't ideal (IMO it's better to use positional args for required arguments), but uniformity is better yet.

          There are some additional changes I'm planning as follow-ons, like changing the setECPolicy API to take a string rather than a byte (which pushes the validation to the NN, and makes it more similar to the SP set API), and also making the EC policy required in set's PB as groundwork for removing the concept of the system default EC policy.

          Show
          andrew.wang Andrew Wang added a comment - Patch attached, I borrowed heavily from StoragePolicyAdmin. I think the SP APIs isn't ideal (IMO it's better to use positional args for required arguments), but uniformity is better yet. There are some additional changes I'm planning as follow-ons, like changing the setECPolicy API to take a string rather than a byte (which pushes the validation to the NN, and makes it more similar to the SP set API), and also making the EC policy required in set 's PB as groundwork for removing the concept of the system default EC policy.
          Hide
          andrew.wang Andrew Wang added a comment -

          Forgot to delete ECCommand, no longer used.

          Show
          andrew.wang Andrew Wang added a comment - Forgot to delete ECCommand, no longer used.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
          0 shelldocs 0m 0s Shelldocs was not available.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 4 new or modified test files.
          0 mvndep 0m 16s Maven dependency ordering for branch
          +1 mvninstall 13m 54s trunk passed
          +1 compile 15m 11s trunk passed
          +1 checkstyle 2m 14s trunk passed
          +1 mvnsite 2m 50s trunk passed
          +1 mvneclipse 0m 51s trunk passed
          +1 findbugs 4m 22s trunk passed
          +1 javadoc 2m 3s trunk passed
          0 mvndep 0m 19s Maven dependency ordering for patch
          +1 mvninstall 1m 51s the patch passed
          +1 compile 12m 43s the patch passed
          +1 javac 12m 43s the patch passed
          +1 checkstyle 2m 8s the patch passed
          +1 mvnsite 2m 20s the patch passed
          +1 mvneclipse 0m 52s the patch passed
          +1 shellcheck 0m 18s There were no new shellcheck issues.
          -1 whitespace 0m 0s The patch has 2 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 4m 7s the patch passed
          +1 javadoc 1m 57s the patch passed
          +1 unit 9m 54s hadoop-common in the patch passed.
          -1 unit 75m 36s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 44s The patch does not generate ASF License warnings.
          156m 29s



          Reason Tests
          Failed junit tests hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11426
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12853360/HDFS-11426.001.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle shellcheck shelldocs xml
          uname Linux e236c90a0a7e 3.13.0-105-generic #152-Ubuntu SMP Fri Dec 2 15:37:11 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / c7a36e6
          Default Java 1.8.0_121
          shellcheck v0.4.5
          findbugs v3.0.0
          whitespace https://builds.apache.org/job/PreCommit-HDFS-Build/18397/artifact/patchprocess/whitespace-eol.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18397/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18397/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18397/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 17s Docker mode activated. 0 shelldocs 0m 0s Shelldocs was not available. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 4 new or modified test files. 0 mvndep 0m 16s Maven dependency ordering for branch +1 mvninstall 13m 54s trunk passed +1 compile 15m 11s trunk passed +1 checkstyle 2m 14s trunk passed +1 mvnsite 2m 50s trunk passed +1 mvneclipse 0m 51s trunk passed +1 findbugs 4m 22s trunk passed +1 javadoc 2m 3s trunk passed 0 mvndep 0m 19s Maven dependency ordering for patch +1 mvninstall 1m 51s the patch passed +1 compile 12m 43s the patch passed +1 javac 12m 43s the patch passed +1 checkstyle 2m 8s the patch passed +1 mvnsite 2m 20s the patch passed +1 mvneclipse 0m 52s the patch passed +1 shellcheck 0m 18s There were no new shellcheck issues. -1 whitespace 0m 0s The patch has 2 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 4m 7s the patch passed +1 javadoc 1m 57s the patch passed +1 unit 9m 54s hadoop-common in the patch passed. -1 unit 75m 36s hadoop-hdfs in the patch failed. +1 asflicense 0m 44s The patch does not generate ASF License warnings. 156m 29s Reason Tests Failed junit tests hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11426 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12853360/HDFS-11426.001.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle shellcheck shelldocs xml uname Linux e236c90a0a7e 3.13.0-105-generic #152-Ubuntu SMP Fri Dec 2 15:37:11 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / c7a36e6 Default Java 1.8.0_121 shellcheck v0.4.5 findbugs v3.0.0 whitespace https://builds.apache.org/job/PreCommit-HDFS-Build/18397/artifact/patchprocess/whitespace-eol.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/18397/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18397/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18397/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 19s Docker mode activated.
          0 shelldocs 0m 0s Shelldocs was not available.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 4 new or modified test files.
          0 mvndep 0m 15s Maven dependency ordering for branch
          +1 mvninstall 12m 40s trunk passed
          +1 compile 13m 51s trunk passed
          +1 checkstyle 1m 58s trunk passed
          +1 mvnsite 2m 13s trunk passed
          +1 mvneclipse 0m 48s trunk passed
          +1 findbugs 3m 43s trunk passed
          +1 javadoc 1m 45s trunk passed
          0 mvndep 0m 17s Maven dependency ordering for patch
          +1 mvninstall 1m 46s the patch passed
          +1 compile 12m 43s the patch passed
          +1 javac 12m 43s the patch passed
          +1 checkstyle 1m 59s the patch passed
          +1 mvnsite 2m 11s the patch passed
          +1 mvneclipse 0m 50s the patch passed
          +1 shellcheck 0m 16s There were no new shellcheck issues.
          -1 whitespace 0m 0s The patch has 2 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 3m 48s the patch passed
          +1 javadoc 1m 45s the patch passed
          +1 unit 10m 16s hadoop-common in the patch passed.
          +1 unit 95m 31s hadoop-hdfs in the patch passed.
          +1 asflicense 0m 40s The patch does not generate ASF License warnings.
          171m 9s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11426
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12853373/HDFS-11426.002.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle shellcheck shelldocs xml
          uname Linux 1b7ced0baa30 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / dbbfcf7
          Default Java 1.8.0_121
          shellcheck v0.4.5
          findbugs v3.0.0
          whitespace https://builds.apache.org/job/PreCommit-HDFS-Build/18398/artifact/patchprocess/whitespace-eol.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18398/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18398/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 19s Docker mode activated. 0 shelldocs 0m 0s Shelldocs was not available. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 4 new or modified test files. 0 mvndep 0m 15s Maven dependency ordering for branch +1 mvninstall 12m 40s trunk passed +1 compile 13m 51s trunk passed +1 checkstyle 1m 58s trunk passed +1 mvnsite 2m 13s trunk passed +1 mvneclipse 0m 48s trunk passed +1 findbugs 3m 43s trunk passed +1 javadoc 1m 45s trunk passed 0 mvndep 0m 17s Maven dependency ordering for patch +1 mvninstall 1m 46s the patch passed +1 compile 12m 43s the patch passed +1 javac 12m 43s the patch passed +1 checkstyle 1m 59s the patch passed +1 mvnsite 2m 11s the patch passed +1 mvneclipse 0m 50s the patch passed +1 shellcheck 0m 16s There were no new shellcheck issues. -1 whitespace 0m 0s The patch has 2 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 3m 48s the patch passed +1 javadoc 1m 45s the patch passed +1 unit 10m 16s hadoop-common in the patch passed. +1 unit 95m 31s hadoop-hdfs in the patch passed. +1 asflicense 0m 40s The patch does not generate ASF License warnings. 171m 9s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11426 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12853373/HDFS-11426.002.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle shellcheck shelldocs xml uname Linux 1b7ced0baa30 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / dbbfcf7 Default Java 1.8.0_121 shellcheck v0.4.5 findbugs v3.0.0 whitespace https://builds.apache.org/job/PreCommit-HDFS-Build/18398/artifact/patchprocess/whitespace-eol.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18398/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18398/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          tasanuma0829 Takanobu Asanuma added a comment -

          Thanks for the work, Andrew Wang! The patch pretty looks good to me. In addition to the whitespace issue, some very minor comments:

          • It adds an unused import statement to AdminHelper.
          • we can remove the import statements of org.apache.commons.logging from CLITestHelper.
          • Slightly odd new lines in ECAdmin.java:
            316:   };
            317:
            318:
            319: }
            
          Show
          tasanuma0829 Takanobu Asanuma added a comment - Thanks for the work, Andrew Wang ! The patch pretty looks good to me. In addition to the whitespace issue, some very minor comments: It adds an unused import statement to AdminHelper . we can remove the import statements of org.apache.commons.logging from CLITestHelper . Slightly odd new lines in ECAdmin.java : 316: }; 317: 318: 319: }
          Hide
          surendrasingh Surendra Singh Lilhore added a comment -

          Thanks Andrew Wang for patch.

          Some comment from my side

          1. I think dfs object should be created after args check. We can avoid extra NN connection in case of wrong argument.

          +      final DistributedFileSystem dfs = AdminHelper.getDFS(conf);
          +      if (args.size() > 0) {
          +        System.err.println(getName() + ": Too many arguments");
          +        return 1;
          +      }
          

          2. One extra space is required after Usage: in GetECPolicyCommand#run()

          +        System.err.println("Please specify the path with -path.\nUsage:" +
          +            getLongUsage());
          

          3. In all three commands dfs object should be created from the path URI. Same thing handled for StoragePolicyAdmin in HDFS-11177

          +      final DistributedFileSystem dfs = AdminHelper.getDFS(conf);
          
          Show
          surendrasingh Surendra Singh Lilhore added a comment - Thanks Andrew Wang for patch. Some comment from my side 1. I think dfs object should be created after args check. We can avoid extra NN connection in case of wrong argument. + final DistributedFileSystem dfs = AdminHelper.getDFS(conf); + if (args.size() > 0) { + System .err.println(getName() + ": Too many arguments" ); + return 1; + } 2. One extra space is required after Usage: in GetECPolicyCommand#run() + System .err.println( "Please specify the path with -path.\nUsage:" + + getLongUsage()); 3. In all three commands dfs object should be created from the path URI. Same thing handled for StoragePolicyAdmin in HDFS-11177 + final DistributedFileSystem dfs = AdminHelper.getDFS(conf);
          Hide
          jojochuang Wei-Chiu Chuang added a comment -

          Hi Andrew,
          Thanks for the patch. I like the refactory proposal, but the patch would require a rebase after HDFS-11405.
          I caught one nit in the patch:

          if (ecPolicy != null) {
                    System.out.println(ecPolicy.toString());
                  } else {
          

          It could be easier to understand with a more verbose output than printing the ec policy.

          Show
          jojochuang Wei-Chiu Chuang added a comment - Hi Andrew, Thanks for the patch. I like the refactory proposal, but the patch would require a rebase after HDFS-11405 . I caught one nit in the patch: if (ecPolicy != null ) { System .out.println(ecPolicy.toString()); } else { It could be easier to understand with a more verbose output than printing the ec policy.
          Hide
          jojochuang Wei-Chiu Chuang added a comment -

          Andrew Wang I caught another issue with the patch. The unset ec policy command works for a directory, not a file.

            /** Command to unset the erasure coding policy set for a file/directory */
            private static class UnsetECPolicyCommand
                implements AdminHelper.Command {
          
              @Override
              public String getName() {
                return "-unsetPolicy";
              }
          
              @Override
              public String getShortUsage() {
                return "[" + getName() + " -path <path>]\n";
              }
          
              @Override
              public String getLongUsage() {
                TableListing listing = AdminHelper.getOptionDescriptionListing();
                listing.addRow("<path>", "The path of the file/directory "
                    + "from which the erasure coding policy will be unset.");
                return getShortUsage() + "\n"
                    + "Unset the erasure coding policy for a file/directory.\n\n"
                    + listing.toString();
              }
          
          Show
          jojochuang Wei-Chiu Chuang added a comment - Andrew Wang I caught another issue with the patch. The unset ec policy command works for a directory, not a file. /** Command to unset the erasure coding policy set for a file/directory */ private static class UnsetECPolicyCommand implements AdminHelper.Command { @Override public String getName() { return "-unsetPolicy" ; } @Override public String getShortUsage() { return "[" + getName() + " -path <path>]\n" ; } @Override public String getLongUsage() { TableListing listing = AdminHelper.getOptionDescriptionListing(); listing.addRow( "<path>" , "The path of the file/directory " + "from which the erasure coding policy will be unset." ); return getShortUsage() + "\n" + "Unset the erasure coding policy for a file/directory.\n\n" + listing.toString(); }
          Hide
          andrew.wang Andrew Wang added a comment -

          Thanks for the reviews Takanobu Asanuma, Surendra Singh Lilhore, Wei-Chiu Chuang. I'm attaching a patch that should address all the review feedback, except as noted below.

          2. One extra space is required after Usage: in GetECPolicyCommand#run()

          I also took the opportunity to fix this missing space in StoragePolicyAdmin, hope that's okay.

          It could be easier to understand with a more verbose output than printing the ec policy.

          Hi Wei-Chiu Chuang, what would you suggest to improve the output? I did make one new change to just print the EC policy name instead of toString since I think it looks better and matches the input to setPolicy:

          -> % hdfs ec -listPolicies
          Erasure Coding Policies:
          	RS-10-4-64k
          	RS-3-2-64k
          	RS-6-3-64k
          	RS-LEGACY-6-3-64k
          	XOR-2-1-64k
          -> % hdfs ec -getPolicy -path hdfs:///ecdir
          RS-3-2-64k
          
          Show
          andrew.wang Andrew Wang added a comment - Thanks for the reviews Takanobu Asanuma , Surendra Singh Lilhore , Wei-Chiu Chuang . I'm attaching a patch that should address all the review feedback, except as noted below. 2. One extra space is required after Usage: in GetECPolicyCommand#run() I also took the opportunity to fix this missing space in StoragePolicyAdmin, hope that's okay. It could be easier to understand with a more verbose output than printing the ec policy. Hi Wei-Chiu Chuang , what would you suggest to improve the output? I did make one new change to just print the EC policy name instead of toString since I think it looks better and matches the input to setPolicy : -> % hdfs ec -listPolicies Erasure Coding Policies: RS-10-4-64k RS-3-2-64k RS-6-3-64k RS-LEGACY-6-3-64k XOR-2-1-64k -> % hdfs ec -getPolicy -path hdfs:///ecdir RS-3-2-64k
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 53s Docker mode activated.
          0 shelldocs 0m 1s Shelldocs was not available.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 4 new or modified test files.
          0 mvndep 2m 12s Maven dependency ordering for branch
          +1 mvninstall 14m 16s trunk passed
          +1 compile 16m 33s trunk passed
          +1 checkstyle 2m 29s trunk passed
          +1 mvnsite 3m 9s trunk passed
          +1 mvneclipse 1m 8s trunk passed
          +1 findbugs 4m 19s trunk passed
          +1 javadoc 2m 28s trunk passed
          0 mvndep 0m 24s Maven dependency ordering for patch
          +1 mvninstall 2m 3s the patch passed
          +1 compile 15m 2s the patch passed
          +1 javac 15m 2s the patch passed
          +1 checkstyle 2m 16s the patch passed
          +1 mvnsite 3m 1s the patch passed
          +1 mvneclipse 1m 3s the patch passed
          +1 shellcheck 0m 21s There were no new shellcheck issues.
          +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 51s the patch passed
          +1 javadoc 2m 17s the patch passed
          -1 unit 10m 53s hadoop-common in the patch failed.
          -1 unit 108m 33s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 43s The patch does not generate ASF License warnings.
          201m 5s



          Reason Tests
          Failed junit tests hadoop.security.TestRaceWhenRelogin
            hadoop.ipc.TestIPC
            hadoop.hdfs.server.datanode.checker.TestThrottledAsyncChecker
            hadoop.hdfs.TestDFSRSDefault10x4StripedOutputStreamWithFailure
            hadoop.hdfs.server.datanode.TestDataNodeMultipleRegistrations
            hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure
            hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting
            hadoop.hdfs.server.datanode.TestDirectoryScanner



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11426
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12853829/HDFS-11426.003.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle shellcheck shelldocs xml
          uname Linux f24873c4cce5 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 003ae00
          Default Java 1.8.0_121
          shellcheck v0.4.5
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18412/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18412/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18412/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18412/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 53s Docker mode activated. 0 shelldocs 0m 1s Shelldocs was not available. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 4 new or modified test files. 0 mvndep 2m 12s Maven dependency ordering for branch +1 mvninstall 14m 16s trunk passed +1 compile 16m 33s trunk passed +1 checkstyle 2m 29s trunk passed +1 mvnsite 3m 9s trunk passed +1 mvneclipse 1m 8s trunk passed +1 findbugs 4m 19s trunk passed +1 javadoc 2m 28s trunk passed 0 mvndep 0m 24s Maven dependency ordering for patch +1 mvninstall 2m 3s the patch passed +1 compile 15m 2s the patch passed +1 javac 15m 2s the patch passed +1 checkstyle 2m 16s the patch passed +1 mvnsite 3m 1s the patch passed +1 mvneclipse 1m 3s the patch passed +1 shellcheck 0m 21s There were no new shellcheck issues. +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 51s the patch passed +1 javadoc 2m 17s the patch passed -1 unit 10m 53s hadoop-common in the patch failed. -1 unit 108m 33s hadoop-hdfs in the patch failed. +1 asflicense 0m 43s The patch does not generate ASF License warnings. 201m 5s Reason Tests Failed junit tests hadoop.security.TestRaceWhenRelogin   hadoop.ipc.TestIPC   hadoop.hdfs.server.datanode.checker.TestThrottledAsyncChecker   hadoop.hdfs.TestDFSRSDefault10x4StripedOutputStreamWithFailure   hadoop.hdfs.server.datanode.TestDataNodeMultipleRegistrations   hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure   hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting   hadoop.hdfs.server.datanode.TestDirectoryScanner Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11426 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12853829/HDFS-11426.003.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle shellcheck shelldocs xml uname Linux f24873c4cce5 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 003ae00 Default Java 1.8.0_121 shellcheck v0.4.5 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/18412/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/18412/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18412/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18412/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jojochuang Wei-Chiu Chuang added a comment - - edited

          Hi Andrew, thanks much for addressing my review in the new patch.
          One more nitpick:

                  ErasureCodingPolicy ecPolicy = dfs.getErasureCodingPolicy(p);
                  if (ecPolicy != null) {
                    System.out.println(ecPolicy.getName());
                  } else {
                    System.out.println("The erasure coding policy of " + path + " is " +
                        "unspecified");
                  }
          

          Looking at FSDirErasureCodingOp#getErasureCodingPolicyForPath, the ecPolicy can be null due to:

          1. the file is not striped
          2. the policy id associated with the file is unsupported
          3. symlink directory (current implementation do not support ecpolicy for symlink dir)
          4. the directory, nor its parents, does not have a ec policy specified.

          It's a little vague saying the ec policy is unspecified considering it can be any of the 4 cases.

          Show
          jojochuang Wei-Chiu Chuang added a comment - - edited Hi Andrew, thanks much for addressing my review in the new patch. One more nitpick: ErasureCodingPolicy ecPolicy = dfs.getErasureCodingPolicy(p); if (ecPolicy != null ) { System .out.println(ecPolicy.getName()); } else { System .out.println( "The erasure coding policy of " + path + " is " + "unspecified" ); } Looking at FSDirErasureCodingOp#getErasureCodingPolicyForPath , the ecPolicy can be null due to: the file is not striped the policy id associated with the file is unsupported symlink directory (current implementation do not support ecpolicy for symlink dir) the directory, nor its parents, does not have a ec policy specified. It's a little vague saying the ec policy is unspecified considering it can be any of the 4 cases.
          Hide
          andrew.wang Andrew Wang added a comment -

          Thanks for the review! I think it's okay to be a little vague here, since getPolicy is just a wrapper for the getECPolicy API. It's not meant to provide fuller information like fsck or oiv, and doing so would require more NN RPCs. It also matches the behavior of the similar storagepolicies CLI command.

          We also provide better error messages for the 1-3 you mention when the user calls setPolicy.

          Show
          andrew.wang Andrew Wang added a comment - Thanks for the review! I think it's okay to be a little vague here, since getPolicy is just a wrapper for the getECPolicy API. It's not meant to provide fuller information like fsck or oiv , and doing so would require more NN RPCs. It also matches the behavior of the similar storagepolicies CLI command. We also provide better error messages for the 1-3 you mention when the user calls setPolicy .
          Hide
          jojochuang Wei-Chiu Chuang added a comment -

          Understood your concern about more NN RPCs. I think that's OK.
          Other than that the rest looks good to me.

          Surendra Singh Lilhore Takanobu Asanuma any more comments? I am +1 pending your comments. Thanks!

          Show
          jojochuang Wei-Chiu Chuang added a comment - Understood your concern about more NN RPCs. I think that's OK. Other than that the rest looks good to me. Surendra Singh Lilhore Takanobu Asanuma any more comments? I am +1 pending your comments. Thanks!
          Hide
          surendrasingh Surendra Singh Lilhore added a comment - - edited

          Sorry but one more comment form my side..
          In SetECPolicyCommand we can avoid one RPC for getErasureCodingPolicies().

          +        ErasureCodingPolicy[] ecPolicies =
          +            dfs.getClient().getErasureCodingPolicies();
          +        for (ErasureCodingPolicy policy : ecPolicies) {
          +          if (ecPolicyName.equals(policy.getName())) {
          +            ecPolicy = policy;
          +            break;
          +          }
          +        }
          +        if (ecPolicy == null) {
          +          StringBuilder sb = new StringBuilder();
          +          sb.append("Policy '");
          +          sb.append(ecPolicyName);
          +          sb.append("' does not match any of the supported policies.");
          +          sb.append(" Please select any one of ");
          +          List<String> ecPolicyNames = new ArrayList<String>();
          +          for (ErasureCodingPolicy policy : ecPolicies) {
          +            ecPolicyNames.add(policy.getName());
          +          }
          +          sb.append(ecPolicyNames);
          +          System.err.println(sb.toString());
          +          return 3;
          +        }
          

          This check already there in Namenode FSDirErasureCodingOp#setErasureCodingPolicyXAttr

                if (!validPolicy) {
                  List<String> ecPolicyNames = new ArrayList<String>();
                  for (ErasureCodingPolicy activePolicy : activePolicies) {
                    ecPolicyNames.add(activePolicy.getName());
                  }
                  throw new HadoopIllegalArgumentException("Policy [ " +
                      ecPolicy.getName() + " ] does not match any of the " +
                      "supported policies. Please select any one of " + ecPolicyNames);
                }
          

          If we change this only one RPC call is required in both the case (Invalid and valid policy)

          Show
          surendrasingh Surendra Singh Lilhore added a comment - - edited Sorry but one more comment form my side.. In SetECPolicyCommand we can avoid one RPC for getErasureCodingPolicies() . + ErasureCodingPolicy[] ecPolicies = + dfs.getClient().getErasureCodingPolicies(); + for (ErasureCodingPolicy policy : ecPolicies) { + if (ecPolicyName.equals(policy.getName())) { + ecPolicy = policy; + break ; + } + } + if (ecPolicy == null ) { + StringBuilder sb = new StringBuilder(); + sb.append( "Policy '" ); + sb.append(ecPolicyName); + sb.append( "' does not match any of the supported policies." ); + sb.append( " Please select any one of " ); + List< String > ecPolicyNames = new ArrayList< String >(); + for (ErasureCodingPolicy policy : ecPolicies) { + ecPolicyNames.add(policy.getName()); + } + sb.append(ecPolicyNames); + System .err.println(sb.toString()); + return 3; + } This check already there in Namenode FSDirErasureCodingOp#setErasureCodingPolicyXAttr if (!validPolicy) { List< String > ecPolicyNames = new ArrayList< String >(); for (ErasureCodingPolicy activePolicy : activePolicies) { ecPolicyNames.add(activePolicy.getName()); } throw new HadoopIllegalArgumentException( "Policy [ " + ecPolicy.getName() + " ] does not match any of the " + "supported policies. Please select any one of " + ecPolicyNames); } If we change this only one RPC call is required in both the case (Invalid and valid policy)
          Hide
          tasanuma0829 Takanobu Asanuma added a comment -

          +1(non-binding) for the latest patch. Thanks for the patch and the discussion!

          Show
          tasanuma0829 Takanobu Asanuma added a comment - +1(non-binding) for the latest patch. Thanks for the patch and the discussion!
          Hide
          andrew.wang Andrew Wang added a comment -

          Hi Surendra Singh Lilhore so your review comment actually relates to HDFS-11428 which I already have a patch up for. setECPolicy requires an ECPolicy object, which we can't get without calling getECPolicies first. HDFS-11428 changes it to take a string like setStoragePolicy which avoids this extra RPC.

          I plan to commit this by EOD based on Wei-chiu's +1, hoping that we can address further issues in follow-on JIRAs.

          Show
          andrew.wang Andrew Wang added a comment - Hi Surendra Singh Lilhore so your review comment actually relates to HDFS-11428 which I already have a patch up for. setECPolicy requires an ECPolicy object, which we can't get without calling getECPolicies first. HDFS-11428 changes it to take a string like setStoragePolicy which avoids this extra RPC. I plan to commit this by EOD based on Wei-chiu's +1, hoping that we can address further issues in follow-on JIRAs.
          Hide
          andrew.wang Andrew Wang added a comment -

          Committed to trunk, thanks for the reviews Wei-chiu, Surendra, Takanobu!

          Show
          andrew.wang Andrew Wang added a comment - Committed to trunk, thanks for the reviews Wei-chiu, Surendra, Takanobu!
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11297 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11297/)
          HDFS-11426. Refactor EC CLI to be similar to storage policies CLI. (wang: rev 132f758e3dbe3a3f11c0d9b2de8edbee594fb475)

          • (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java
          • (delete) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/erasurecode/ECCli.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/resources/testErasureCodingConf.xml
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/cli/util/ErasureCodingCliCmdExecutor.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/StoragePolicyAdmin.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/bin/hdfs
          • (add) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/ECAdmin.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/site/markdown/HDFSErasureCoding.md
          • (delete) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/erasurecode/ECCommand.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/cli/CLITestCmdErasureCoding.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11297 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11297/ ) HDFS-11426 . Refactor EC CLI to be similar to storage policies CLI. (wang: rev 132f758e3dbe3a3f11c0d9b2de8edbee594fb475) (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/cli/CLITestHelper.java (delete) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/erasurecode/ECCli.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/resources/testErasureCodingConf.xml (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/cli/util/ErasureCodingCliCmdExecutor.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/StoragePolicyAdmin.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/bin/hdfs (add) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/ECAdmin.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/site/markdown/HDFSErasureCoding.md (delete) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/erasurecode/ECCommand.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/cli/CLITestCmdErasureCoding.java

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development