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

Add builder-based create API to FileSystem

    Details

      Description

      FileSystem class supports multiple create functions to help user create file. Some create functions has many parameters, it's hard for user to exactly remember these parameters and their orders. This task is to add builder based create functions to help user more easily create file.

      1. HDFS-11170-00.patch
        13 kB
        Wei Zhou
      2. HDFS-11170-01.patch
        14 kB
        Wei Zhou
      3. HDFS-11170-02.patch
        18 kB
        SammiChen
      4. HDFS-11170-03.patch
        19 kB
        SammiChen
      5. HDFS-11170-04.patch
        26 kB
        SammiChen
      6. HDFS-11170-05.patch
        21 kB
        SammiChen
      7. HDFS-11170-06.patch
        19 kB
        SammiChen
      8. HDFS-11170-07.patch
        19 kB
        SammiChen
      9. HDFS-11170-08.patch
        19 kB
        SammiChen
      10. HDFS-11170-branch-2.001.patch
        19 kB
        SammiChen

        Issue Links

          Activity

          Hide
          stevel@apache.org Steve Loughran added a comment -

          When people make changes to the FileSystem class, can they

          1. update what they actually do in the Filesystem specification markdown files, so we have a full declaration of what's meant to happen. Yes, it's extra work, but it forces you to think through what cross-FS implementation expectations are, rather than just have them do whatever HDFS does
          2. write contract tests so that the downstream filesystems can all share
          3. and when we do a new operation, we get to revisit what is wrong with the current one, rather than just repeat the old behavior.

          One way to to assist this is to just warn me that this is going on, and while I won't do the work, I can help.

          Now some comments on the code of various levels of importance

          • Why is FileContext left out?
          • newFSDataOutputStreamBuilder() isn't a good name. It describers the return value, but not what it is trying to do, which is `createFile()`
          • If you look at what is wrong with FileSystem.create();, the fact that callers can expect a directory to be created is a problem; it's very expensive on object stores where we have to walk the tree and look for things above (files). If you look at the codepaths of how create() is used, most people create the parent dir anyway, they don't relay on this feature. So we could make it yet another builder option ("createParentDirs") and have people explkicitly create it if they want, leave it off by default (yes, we'll have to tweak filesystem.create somehow, but we can do that across the board in our own code)

          & on the tests

          • I don't think the HDFS test case needs to bring up a test cluster just for one test suite; it's slow. Again, Moving this into a new Contract test and then having the HDFS be one of the concrete implementations will sort this out. If added to AbstractContractCreateTest then all filesystems will get this test added for free, which is what is required if they are all expected to support the API.
          • your asserts all get their expected/actual values wrong. Flip the order

          I'm going to create a new JIRA to finish up the issues,. I really do want this to be stable. At the very least, release notes must indicate the fact that this is still stabilising,

          Sorry for getting in so late & causing trouble, but I'd only just noticed that a change had happened to FileSystem.java after the fact.

          Show
          stevel@apache.org Steve Loughran added a comment - When people make changes to the FileSystem class, can they update what they actually do in the Filesystem specification markdown files, so we have a full declaration of what's meant to happen. Yes, it's extra work, but it forces you to think through what cross-FS implementation expectations are, rather than just have them do whatever HDFS does write contract tests so that the downstream filesystems can all share and when we do a new operation, we get to revisit what is wrong with the current one, rather than just repeat the old behavior. One way to to assist this is to just warn me that this is going on, and while I won't do the work, I can help. Now some comments on the code of various levels of importance Why is FileContext left out? newFSDataOutputStreamBuilder() isn't a good name. It describers the return value, but not what it is trying to do, which is `createFile()` If you look at what is wrong with FileSystem.create(); , the fact that callers can expect a directory to be created is a problem; it's very expensive on object stores where we have to walk the tree and look for things above (files). If you look at the codepaths of how create() is used, most people create the parent dir anyway, they don't relay on this feature. So we could make it yet another builder option ("createParentDirs") and have people explkicitly create it if they want, leave it off by default (yes, we'll have to tweak filesystem.create somehow, but we can do that across the board in our own code) & on the tests I don't think the HDFS test case needs to bring up a test cluster just for one test suite; it's slow. Again, Moving this into a new Contract test and then having the HDFS be one of the concrete implementations will sort this out. If added to AbstractContractCreateTest then all filesystems will get this test added for free, which is what is required if they are all expected to support the API. your asserts all get their expected/actual values wrong. Flip the order I'm going to create a new JIRA to finish up the issues,. I really do want this to be stable. At the very least, release notes must indicate the fact that this is still stabilising, Sorry for getting in so late & causing trouble, but I'd only just noticed that a change had happened to FileSystem.java after the fact.
          Hide
          andrew.wang Andrew Wang added a comment -

          Thanks again Sammi, committed branch-2 patch to branch-2.

          Show
          andrew.wang Andrew Wang added a comment - Thanks again Sammi, committed branch-2 patch to branch-2.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 14m 18s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
          0 mvndep 1m 49s Maven dependency ordering for branch
          +1 mvninstall 8m 31s branch-2 passed
          +1 compile 6m 3s branch-2 passed with JDK v1.8.0_121
          +1 compile 6m 37s branch-2 passed with JDK v1.7.0_121
          +1 checkstyle 1m 30s branch-2 passed
          +1 mvnsite 2m 25s branch-2 passed
          +1 mvneclipse 0m 45s branch-2 passed
          +1 findbugs 5m 12s branch-2 passed
          +1 javadoc 2m 9s branch-2 passed with JDK v1.8.0_121
          +1 javadoc 3m 1s branch-2 passed with JDK v1.7.0_121
          0 mvndep 0m 14s Maven dependency ordering for patch
          +1 mvninstall 1m 55s the patch passed
          +1 compile 7m 4s the patch passed with JDK v1.8.0_121
          +1 javac 7m 4s the patch passed
          +1 compile 7m 1s the patch passed with JDK v1.7.0_121
          +1 javac 7m 1s the patch passed
          -0 checkstyle 1m 27s root: The patch generated 1 new + 236 unchanged - 1 fixed = 237 total (was 237)
          +1 mvnsite 2m 20s the patch passed
          +1 mvneclipse 0m 40s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 5m 58s the patch passed
          +1 javadoc 1m 59s the patch passed with JDK v1.8.0_121
          +1 javadoc 2m 56s the patch passed with JDK v1.7.0_121
          +1 unit 8m 28s hadoop-common in the patch passed with JDK v1.7.0_121.
          +1 unit 1m 4s hadoop-hdfs-client in the patch passed with JDK v1.7.0_121.
          -1 unit 54m 33s hadoop-hdfs in the patch failed with JDK v1.7.0_121.
          +1 asflicense 0m 25s The patch does not generate ASF License warnings.
          223m 51s



          Reason Tests
          JDK v1.8.0_121 Failed junit tests hadoop.metrics2.sink.TestRollingFileSystemSinkWithHdfs
            hadoop.hdfs.server.datanode.TestBPOfferService
          JDK v1.8.0_121 Timed out junit tests org.apache.hadoop.hdfs.TestLeaseRecovery2
          JDK v1.7.0_121 Failed junit tests hadoop.hdfs.server.blockmanagement.TestReplicationPolicyWithUpgradeDomain



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:b59b8b7
          JIRA Issue HDFS-11170
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12860600/HDFS-11170-branch-2.001.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 7b05b2f01b34 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 branch-2 / 675bc97
          Default Java 1.7.0_121
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_121 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18848/artifact/patchprocess/diff-checkstyle-root.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18848/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_121.txt
          JDK v1.7.0_121 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18848/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/18848/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 14m 18s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. 0 mvndep 1m 49s Maven dependency ordering for branch +1 mvninstall 8m 31s branch-2 passed +1 compile 6m 3s branch-2 passed with JDK v1.8.0_121 +1 compile 6m 37s branch-2 passed with JDK v1.7.0_121 +1 checkstyle 1m 30s branch-2 passed +1 mvnsite 2m 25s branch-2 passed +1 mvneclipse 0m 45s branch-2 passed +1 findbugs 5m 12s branch-2 passed +1 javadoc 2m 9s branch-2 passed with JDK v1.8.0_121 +1 javadoc 3m 1s branch-2 passed with JDK v1.7.0_121 0 mvndep 0m 14s Maven dependency ordering for patch +1 mvninstall 1m 55s the patch passed +1 compile 7m 4s the patch passed with JDK v1.8.0_121 +1 javac 7m 4s the patch passed +1 compile 7m 1s the patch passed with JDK v1.7.0_121 +1 javac 7m 1s the patch passed -0 checkstyle 1m 27s root: The patch generated 1 new + 236 unchanged - 1 fixed = 237 total (was 237) +1 mvnsite 2m 20s the patch passed +1 mvneclipse 0m 40s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 5m 58s the patch passed +1 javadoc 1m 59s the patch passed with JDK v1.8.0_121 +1 javadoc 2m 56s the patch passed with JDK v1.7.0_121 +1 unit 8m 28s hadoop-common in the patch passed with JDK v1.7.0_121. +1 unit 1m 4s hadoop-hdfs-client in the patch passed with JDK v1.7.0_121. -1 unit 54m 33s hadoop-hdfs in the patch failed with JDK v1.7.0_121. +1 asflicense 0m 25s The patch does not generate ASF License warnings. 223m 51s Reason Tests JDK v1.8.0_121 Failed junit tests hadoop.metrics2.sink.TestRollingFileSystemSinkWithHdfs   hadoop.hdfs.server.datanode.TestBPOfferService JDK v1.8.0_121 Timed out junit tests org.apache.hadoop.hdfs.TestLeaseRecovery2 JDK v1.7.0_121 Failed junit tests hadoop.hdfs.server.blockmanagement.TestReplicationPolicyWithUpgradeDomain Subsystem Report/Notes Docker Image:yetus/hadoop:b59b8b7 JIRA Issue HDFS-11170 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12860600/HDFS-11170-branch-2.001.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 7b05b2f01b34 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 branch-2 / 675bc97 Default Java 1.7.0_121 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_121 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18848/artifact/patchprocess/diff-checkstyle-root.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/18848/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_121.txt JDK v1.7.0_121 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18848/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/18848/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          Sammi SammiChen added a comment -

          Patch for branch-2

          Show
          Sammi SammiChen added a comment - Patch for branch-2
          Hide
          Sammi SammiChen added a comment -

          Thanks Andrew for review and commit the patch! Sure, I will upload a patch for branch-2 later.

          Show
          Sammi SammiChen added a comment - Thanks Andrew for review and commit the patch! Sure, I will upload a patch for branch-2 later.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11457 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11457/)
          HDFS-11170. Add builder-based create API to FileSystem. Contributed by (wang: rev 332a997e10cca88d9ab3aa8252102366b628eaec)

          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFavoredNodesEndToEnd.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDistributedFileSystem.java
          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/HarFileSystem.java
          • (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestLocalFileSystem.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FilterFileSystem.java
          • (add) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FSDataOutputStreamBuilder.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11457 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11457/ ) HDFS-11170 . Add builder-based create API to FileSystem. Contributed by (wang: rev 332a997e10cca88d9ab3aa8252102366b628eaec) (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFavoredNodesEndToEnd.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDistributedFileSystem.java (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/HarFileSystem.java (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestLocalFileSystem.java (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FilterFileSystem.java (add) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FSDataOutputStreamBuilder.java
          Hide
          andrew.wang Andrew Wang added a comment -

          Reopen issue for branch-2 backport.

          Show
          andrew.wang Andrew Wang added a comment - Reopen issue for branch-2 backport.
          Hide
          andrew.wang Andrew Wang added a comment -

          Committed to trunk, thanks Sammi!

          Do you mind making a patch for branch-2 as well? Backport wasn't quite clean.

          Show
          andrew.wang Andrew Wang added a comment - Committed to trunk, thanks Sammi! Do you mind making a patch for branch-2 as well? Backport wasn't quite clean.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 12s 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 48s Maven dependency ordering for branch
          +1 mvninstall 14m 35s trunk passed
          +1 compile 15m 39s trunk passed
          +1 checkstyle 1m 57s trunk passed
          +1 mvnsite 2m 41s trunk passed
          +1 mvneclipse 0m 58s trunk passed
          +1 findbugs 4m 47s trunk passed
          +1 javadoc 2m 1s trunk passed
          0 mvndep 0m 13s Maven dependency ordering for patch
          +1 mvninstall 1m 54s the patch passed
          +1 compile 14m 11s the patch passed
          +1 javac 14m 11s the patch passed
          -0 checkstyle 1m 56s root: The patch generated 1 new + 254 unchanged - 1 fixed = 255 total (was 255)
          +1 mvnsite 2m 38s the patch passed
          +1 mvneclipse 0m 58s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 5m 17s the patch passed
          +1 javadoc 2m 1s the patch passed
          +1 unit 7m 42s hadoop-common in the patch passed.
          +1 unit 1m 8s hadoop-hdfs-client in the patch passed.
          -1 unit 68m 49s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 36s The patch does not generate ASF License warnings.
          153m 21s



          Reason Tests
          Failed junit tests hadoop.hdfs.TestMaintenanceState
            hadoop.hdfs.TestDFSStripedOutputStreamWithFailure010
            hadoop.hdfs.server.namenode.TestNamenodeCapacityReport



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11170
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12860276/HDFS-11170-08.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux beed3bc6e768 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 / 1280155
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18820/artifact/patchprocess/diff-checkstyle-root.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18820/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18820/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/18820/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 12s 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 48s Maven dependency ordering for branch +1 mvninstall 14m 35s trunk passed +1 compile 15m 39s trunk passed +1 checkstyle 1m 57s trunk passed +1 mvnsite 2m 41s trunk passed +1 mvneclipse 0m 58s trunk passed +1 findbugs 4m 47s trunk passed +1 javadoc 2m 1s trunk passed 0 mvndep 0m 13s Maven dependency ordering for patch +1 mvninstall 1m 54s the patch passed +1 compile 14m 11s the patch passed +1 javac 14m 11s the patch passed -0 checkstyle 1m 56s root: The patch generated 1 new + 254 unchanged - 1 fixed = 255 total (was 255) +1 mvnsite 2m 38s the patch passed +1 mvneclipse 0m 58s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 5m 17s the patch passed +1 javadoc 2m 1s the patch passed +1 unit 7m 42s hadoop-common in the patch passed. +1 unit 1m 8s hadoop-hdfs-client in the patch passed. -1 unit 68m 49s hadoop-hdfs in the patch failed. +1 asflicense 0m 36s The patch does not generate ASF License warnings. 153m 21s Reason Tests Failed junit tests hadoop.hdfs.TestMaintenanceState   hadoop.hdfs.TestDFSStripedOutputStreamWithFailure010   hadoop.hdfs.server.namenode.TestNamenodeCapacityReport Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11170 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12860276/HDFS-11170-08.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux beed3bc6e768 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 / 1280155 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18820/artifact/patchprocess/diff-checkstyle-root.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/18820/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18820/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/18820/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          Sammi SammiChen added a comment - - edited

          Hi Andrew, thanks for the comments! I will refine the patch. Use the "final" keyword doesn't suppress the firebug from report the issue. After do some investigation, I think I have to go back to use clone in getter and setter of "favoredNodes".

          Show
          Sammi SammiChen added a comment - - edited Hi Andrew, thanks for the comments! I will refine the patch. Use the "final" keyword doesn't suppress the firebug from report the issue. After do some investigation, I think I have to go back to use clone in getter and setter of "favoredNodes".
          Hide
          andrew.wang Andrew Wang added a comment -

          Hi Sammi, thanks for the rev!

          The reason I think is that leave the setDefaultValue right before the create call to save the unnecessary default value assignment if API caller will call the specific setter to set the value.

          I don't think this causes any additional assignment, since the getters are only called once (at build time), and inline the same if statements as what's in setDefaultValue. My sample code looked like this:

            protected int getBufferSize() {
              if (bufferSize == null) {
                return fs.getConf().getInt(IO_FILE_BUFFER_SIZE_KEY,
                    IO_FILE_BUFFER_SIZE_DEFAULT);
              }
              return bufferSize;
            }
          

          Returning directly might also be a bit more efficient by not calling the setter (current setDefaultValue behavior).

          From the point that unit tests as example code, would it be better to keep a distinct test function, so user can easily find it by refer to the function name? Also, I went through the test function name list of TestDistributedFileSystem, all function name carries their target clearly.

          Sure, it's okay to leave it as is. I agree that this is easier to find and a good standalone example.

          Show
          andrew.wang Andrew Wang added a comment - Hi Sammi, thanks for the rev! The reason I think is that leave the setDefaultValue right before the create call to save the unnecessary default value assignment if API caller will call the specific setter to set the value. I don't think this causes any additional assignment, since the getters are only called once (at build time), and inline the same if statements as what's in setDefaultValue . My sample code looked like this: protected int getBufferSize() { if (bufferSize == null ) { return fs.getConf().getInt(IO_FILE_BUFFER_SIZE_KEY, IO_FILE_BUFFER_SIZE_DEFAULT); } return bufferSize; } Returning directly might also be a bit more efficient by not calling the setter (current setDefaultValue behavior). From the point that unit tests as example code, would it be better to keep a distinct test function, so user can easily find it by refer to the function name? Also, I went through the test function name list of TestDistributedFileSystem, all function name carries their target clearly. Sure, it's okay to leave it as is. I agree that this is easier to find and a good standalone example.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 23s 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 50s Maven dependency ordering for branch
          +1 mvninstall 14m 48s trunk passed
          +1 compile 23m 26s trunk passed
          +1 checkstyle 2m 6s trunk passed
          +1 mvnsite 3m 20s trunk passed
          +1 mvneclipse 1m 4s trunk passed
          +1 findbugs 5m 50s trunk passed
          +1 javadoc 2m 24s trunk passed
          0 mvndep 0m 17s Maven dependency ordering for patch
          +1 mvninstall 2m 26s the patch passed
          +1 compile 18m 46s the patch passed
          +1 javac 18m 46s the patch passed
          -0 checkstyle 2m 22s root: The patch generated 1 new + 253 unchanged - 1 fixed = 254 total (was 254)
          +1 mvnsite 3m 25s the patch passed
          +1 mvneclipse 1m 9s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          -1 findbugs 2m 13s hadoop-hdfs-project/hadoop-hdfs-client generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
          +1 javadoc 2m 34s the patch passed
          -1 unit 8m 38s hadoop-common in the patch failed.
          +1 unit 1m 17s hadoop-hdfs-client in the patch passed.
          -1 unit 78m 56s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 38s The patch does not generate ASF License warnings.
          184m 14s



          Reason Tests
          FindBugs module:hadoop-hdfs-project/hadoop-hdfs-client
            org.apache.hadoop.hdfs.DistributedFileSystem$HdfsDataOutputStreamBuilder.setFavoredNodes(InetSocketAddress[]) may expose internal representation by storing an externally mutable object into DistributedFileSystem$HdfsDataOutputStreamBuilder.favoredNodes At DistributedFileSystem.java:by storing an externally mutable object into DistributedFileSystem$HdfsDataOutputStreamBuilder.favoredNodes At DistributedFileSystem.java:[line 2652]
          Failed junit tests hadoop.net.TestDNS
            hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11170
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12860112/HDFS-11170-07.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 95102a33a29b 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 / 59d6925
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18812/artifact/patchprocess/diff-checkstyle-root.txt
          findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/18812/artifact/patchprocess/new-findbugs-hadoop-hdfs-project_hadoop-hdfs-client.html
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18812/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18812/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18812/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/18812/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 23s 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 50s Maven dependency ordering for branch +1 mvninstall 14m 48s trunk passed +1 compile 23m 26s trunk passed +1 checkstyle 2m 6s trunk passed +1 mvnsite 3m 20s trunk passed +1 mvneclipse 1m 4s trunk passed +1 findbugs 5m 50s trunk passed +1 javadoc 2m 24s trunk passed 0 mvndep 0m 17s Maven dependency ordering for patch +1 mvninstall 2m 26s the patch passed +1 compile 18m 46s the patch passed +1 javac 18m 46s the patch passed -0 checkstyle 2m 22s root: The patch generated 1 new + 253 unchanged - 1 fixed = 254 total (was 254) +1 mvnsite 3m 25s the patch passed +1 mvneclipse 1m 9s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. -1 findbugs 2m 13s hadoop-hdfs-project/hadoop-hdfs-client generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) +1 javadoc 2m 34s the patch passed -1 unit 8m 38s hadoop-common in the patch failed. +1 unit 1m 17s hadoop-hdfs-client in the patch passed. -1 unit 78m 56s hadoop-hdfs in the patch failed. +1 asflicense 0m 38s The patch does not generate ASF License warnings. 184m 14s Reason Tests FindBugs module:hadoop-hdfs-project/hadoop-hdfs-client   org.apache.hadoop.hdfs.DistributedFileSystem$HdfsDataOutputStreamBuilder.setFavoredNodes(InetSocketAddress[]) may expose internal representation by storing an externally mutable object into DistributedFileSystem$HdfsDataOutputStreamBuilder.favoredNodes At DistributedFileSystem.java:by storing an externally mutable object into DistributedFileSystem$HdfsDataOutputStreamBuilder.favoredNodes At DistributedFileSystem.java: [line 2652] Failed junit tests hadoop.net.TestDNS   hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11170 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12860112/HDFS-11170-07.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 95102a33a29b 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 / 59d6925 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18812/artifact/patchprocess/diff-checkstyle-root.txt findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/18812/artifact/patchprocess/new-findbugs-hadoop-hdfs-project_hadoop-hdfs-client.html unit https://builds.apache.org/job/PreCommit-HDFS-Build/18812/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/18812/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18812/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/18812/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          Sammi SammiChen added a comment -

          Address firebug issue

          Show
          Sammi SammiChen added a comment - Address firebug issue
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 13s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
          0 mvndep 1m 48s Maven dependency ordering for branch
          +1 mvninstall 13m 21s trunk passed
          +1 compile 21m 15s trunk passed
          +1 checkstyle 1m 56s trunk passed
          +1 mvnsite 2m 41s trunk passed
          +1 mvneclipse 0m 57s trunk passed
          +1 findbugs 4m 47s trunk passed
          +1 javadoc 2m 0s trunk passed
          0 mvndep 0m 14s Maven dependency ordering for patch
          +1 mvninstall 1m 54s the patch passed
          +1 compile 16m 25s the patch passed
          +1 javac 16m 25s the patch passed
          -0 checkstyle 1m 57s root: The patch generated 1 new + 254 unchanged - 1 fixed = 255 total (was 255)
          +1 mvnsite 2m 35s the patch passed
          +1 mvneclipse 0m 57s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          -1 findbugs 1m 44s hadoop-hdfs-project/hadoop-hdfs-client generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
          +1 javadoc 2m 2s the patch passed
          -1 unit 7m 14s hadoop-common in the patch failed.
          +1 unit 1m 4s hadoop-hdfs-client in the patch passed.
          -1 unit 73m 11s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 35s The patch does not generate ASF License warnings.
          163m 47s



          Reason Tests
          FindBugs module:hadoop-hdfs-project/hadoop-hdfs-client
            org.apache.hadoop.hdfs.DistributedFileSystem$HdfsDataOutputStreamBuilder.setFavoredNodes(InetSocketAddress[]) may expose internal representation by storing an externally mutable object into DistributedFileSystem$HdfsDataOutputStreamBuilder.favoredNodes At DistributedFileSystem.java:by storing an externally mutable object into DistributedFileSystem$HdfsDataOutputStreamBuilder.favoredNodes At DistributedFileSystem.java:[line 2652]
          Failed junit tests hadoop.fs.TestLocalFileSystem
            hadoop.hdfs.server.namenode.ha.TestHAMetrics
            hadoop.hdfs.server.balancer.TestBalancer
            hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11170
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12860081/HDFS-11170-06.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux cdf2c9ed2dbc 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 / 59d6925
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18809/artifact/patchprocess/diff-checkstyle-root.txt
          findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/18809/artifact/patchprocess/new-findbugs-hadoop-hdfs-project_hadoop-hdfs-client.html
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18809/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18809/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18809/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/18809/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 13s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. 0 mvndep 1m 48s Maven dependency ordering for branch +1 mvninstall 13m 21s trunk passed +1 compile 21m 15s trunk passed +1 checkstyle 1m 56s trunk passed +1 mvnsite 2m 41s trunk passed +1 mvneclipse 0m 57s trunk passed +1 findbugs 4m 47s trunk passed +1 javadoc 2m 0s trunk passed 0 mvndep 0m 14s Maven dependency ordering for patch +1 mvninstall 1m 54s the patch passed +1 compile 16m 25s the patch passed +1 javac 16m 25s the patch passed -0 checkstyle 1m 57s root: The patch generated 1 new + 254 unchanged - 1 fixed = 255 total (was 255) +1 mvnsite 2m 35s the patch passed +1 mvneclipse 0m 57s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. -1 findbugs 1m 44s hadoop-hdfs-project/hadoop-hdfs-client generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) +1 javadoc 2m 2s the patch passed -1 unit 7m 14s hadoop-common in the patch failed. +1 unit 1m 4s hadoop-hdfs-client in the patch passed. -1 unit 73m 11s hadoop-hdfs in the patch failed. +1 asflicense 0m 35s The patch does not generate ASF License warnings. 163m 47s Reason Tests FindBugs module:hadoop-hdfs-project/hadoop-hdfs-client   org.apache.hadoop.hdfs.DistributedFileSystem$HdfsDataOutputStreamBuilder.setFavoredNodes(InetSocketAddress[]) may expose internal representation by storing an externally mutable object into DistributedFileSystem$HdfsDataOutputStreamBuilder.favoredNodes At DistributedFileSystem.java:by storing an externally mutable object into DistributedFileSystem$HdfsDataOutputStreamBuilder.favoredNodes At DistributedFileSystem.java: [line 2652] Failed junit tests hadoop.fs.TestLocalFileSystem   hadoop.hdfs.server.namenode.ha.TestHAMetrics   hadoop.hdfs.server.balancer.TestBalancer   hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11170 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12860081/HDFS-11170-06.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux cdf2c9ed2dbc 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 / 59d6925 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18809/artifact/patchprocess/diff-checkstyle-root.txt findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/18809/artifact/patchprocess/new-findbugs-hadoop-hdfs-project_hadoop-hdfs-client.html unit https://builds.apache.org/job/PreCommit-HDFS-Build/18809/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/18809/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18809/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/18809/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 for the comments. I will upload a new patch based on the suggestions. I would like to further discuss 2 items.

          I recommended earlier to embed the default logic into the getters, since it lets you avoid doing a separate setDefaultValue call. Any reason not to do this?

          The reason I think is that leave the setDefaultValue right before the create call to save the unnecessary default value assignment if API caller will call the specific setter to set the value.

          We could probably include this basic functionality test in an existing testcase to save a minicluster (which takes a few seconds to start and stop)

          From the point that unit tests as example code, would it be better to keep a distinct test function, so user can easily find it by refer to the function name? Also, I went through the test function name list of TestDistributedFileSystem, all function name carries their target clearly.

          Show
          Sammi SammiChen added a comment - Thanks Andrew for the comments. I will upload a new patch based on the suggestions. I would like to further discuss 2 items. I recommended earlier to embed the default logic into the getters, since it lets you avoid doing a separate setDefaultValue call. Any reason not to do this? The reason I think is that leave the setDefaultValue right before the create call to save the unnecessary default value assignment if API caller will call the specific setter to set the value. We could probably include this basic functionality test in an existing testcase to save a minicluster (which takes a few seconds to start and stop) From the point that unit tests as example code, would it be better to keep a distinct test function, so user can easily find it by refer to the function name? Also, I went through the test function name list of TestDistributedFileSystem , all function name carries their target clearly.
          Hide
          andrew.wang Andrew Wang added a comment -

          Hi Sammi, thanks for revving, looks pretty close to me,

          • DFS, do we need to clone when setting/getting favored nodes? I think users would be okay with a shallow copy since builders are almost always short-lived.
          • I recommended earlier to embed the default logic into the getters, since it lets you avoid doing a separate setDefaultValue call. Any reason not to do this?
          • Some unused imports and checkstyles to clean up
          • Since valid values of permission and flags cannot be null, shall we add Precondition.checkArgument checks to their setters?

          Tests

          • I think we can also replace the file contents checks with ContractTestUtils#verifyFileContents or similar
          • There's no need to catch unexpected exceptions just to rethrow them or fail. TestDFS and TestLFS do this.

          TestDFS:

          • typo "an file" -> "a file"
          • We could probably include this basic functionality test in an existing testcase to save a minicluster (which takes a few seconds to start and stop)

          This is a minor nit, but part of the joy of a builder is how it can be chained. We can shorten this in TestFavoredNodesEndToEnd (and probably elsewhere):

                DistributedFileSystem.HdfsDataOutputStreamBuilder b =
                    dfs.newFSDataOutputStreamBuilder(p);
                b.setFavoredNodes(dns);
                FSDataOutputStream out = b.build();
          

          to

                FSDataOutputStream out = dfs.newFSDataOutputStreamBuilder(p);
                    .setFavoredNodes(dns)
                    .build();
          

          Since the unit tests are often looked to for example code, it'd be nice to clean these up.

          Show
          andrew.wang Andrew Wang added a comment - Hi Sammi, thanks for revving, looks pretty close to me, DFS, do we need to clone when setting/getting favored nodes? I think users would be okay with a shallow copy since builders are almost always short-lived. I recommended earlier to embed the default logic into the getters, since it lets you avoid doing a separate setDefaultValue call. Any reason not to do this? Some unused imports and checkstyles to clean up Since valid values of permission and flags cannot be null, shall we add Precondition.checkArgument checks to their setters? Tests I think we can also replace the file contents checks with ContractTestUtils#verifyFileContents or similar There's no need to catch unexpected exceptions just to rethrow them or fail. TestDFS and TestLFS do this. TestDFS: typo "an file" -> "a file" We could probably include this basic functionality test in an existing testcase to save a minicluster (which takes a few seconds to start and stop) This is a minor nit, but part of the joy of a builder is how it can be chained. We can shorten this in TestFavoredNodesEndToEnd (and probably elsewhere): DistributedFileSystem.HdfsDataOutputStreamBuilder b = dfs.newFSDataOutputStreamBuilder(p); b.setFavoredNodes(dns); FSDataOutputStream out = b.build(); to FSDataOutputStream out = dfs.newFSDataOutputStreamBuilder(p); .setFavoredNodes(dns) .build(); Since the unit tests are often looked to for example code, it'd be nice to clean these up.
          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 3 new or modified test files.
          0 mvndep 2m 0s Maven dependency ordering for branch
          +1 mvninstall 12m 32s trunk passed
          +1 compile 20m 52s trunk passed
          +1 checkstyle 1m 53s trunk passed
          +1 mvnsite 2m 41s trunk passed
          +1 mvneclipse 0m 57s trunk passed
          +1 findbugs 4m 47s trunk passed
          +1 javadoc 1m 59s trunk passed
          0 mvndep 0m 14s Maven dependency ordering for patch
          +1 mvninstall 2m 1s the patch passed
          +1 compile 16m 32s the patch passed
          +1 javac 16m 32s the patch passed
          -0 checkstyle 2m 0s root: The patch generated 3 new + 253 unchanged - 1 fixed = 256 total (was 254)
          +1 mvnsite 2m 35s the patch passed
          +1 mvneclipse 0m 58s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 5m 21s the patch passed
          +1 javadoc 2m 2s the patch passed
          +1 unit 8m 32s hadoop-common in the patch passed.
          +1 unit 1m 3s hadoop-hdfs-client in the patch passed.
          -1 unit 68m 1s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 33s The patch does not generate ASF License warnings.
          159m 10s



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



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11170
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12859728/HDFS-11170-05.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 49a05fcacd2e 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 3b908f7
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18785/artifact/patchprocess/diff-checkstyle-root.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18785/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18785/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/18785/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 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 3 new or modified test files. 0 mvndep 2m 0s Maven dependency ordering for branch +1 mvninstall 12m 32s trunk passed +1 compile 20m 52s trunk passed +1 checkstyle 1m 53s trunk passed +1 mvnsite 2m 41s trunk passed +1 mvneclipse 0m 57s trunk passed +1 findbugs 4m 47s trunk passed +1 javadoc 1m 59s trunk passed 0 mvndep 0m 14s Maven dependency ordering for patch +1 mvninstall 2m 1s the patch passed +1 compile 16m 32s the patch passed +1 javac 16m 32s the patch passed -0 checkstyle 2m 0s root: The patch generated 3 new + 253 unchanged - 1 fixed = 256 total (was 254) +1 mvnsite 2m 35s the patch passed +1 mvneclipse 0m 58s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 5m 21s the patch passed +1 javadoc 2m 2s the patch passed +1 unit 8m 32s hadoop-common in the patch passed. +1 unit 1m 3s hadoop-hdfs-client in the patch passed. -1 unit 68m 1s hadoop-hdfs in the patch failed. +1 asflicense 0m 33s The patch does not generate ASF License warnings. 159m 10s Reason Tests Failed junit tests hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11170 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12859728/HDFS-11170-05.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 49a05fcacd2e 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 3b908f7 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18785/artifact/patchprocess/diff-checkstyle-root.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/18785/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18785/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/18785/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 22s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
          0 mvndep 1m 50s Maven dependency ordering for branch
          +1 mvninstall 11m 55s trunk passed
          +1 compile 19m 54s trunk passed
          +1 checkstyle 1m 52s trunk passed
          +1 mvnsite 2m 36s trunk passed
          +1 mvneclipse 1m 4s trunk passed
          +1 findbugs 4m 31s trunk passed
          +1 javadoc 2m 8s trunk passed
          0 mvndep 0m 13s Maven dependency ordering for patch
          +1 mvninstall 1m 44s the patch passed
          +1 compile 15m 2s the patch passed
          +1 javac 15m 2s the patch passed
          -0 checkstyle 1m 52s root: The patch generated 4 new + 253 unchanged - 1 fixed = 257 total (was 254)
          +1 mvnsite 2m 31s the patch passed
          +1 mvneclipse 1m 3s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 4m 54s the patch passed
          +1 javadoc 2m 6s the patch passed
          -1 unit 7m 1s hadoop-common in the patch failed.
          +1 unit 1m 5s hadoop-hdfs-client in the patch passed.
          -1 unit 62m 39s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 37s The patch does not generate ASF License warnings.
          148m 30s



          Reason Tests
          Failed junit tests hadoop.security.TestShellBasedUnixGroupsMapping
            hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure
            hadoop.hdfs.server.datanode.checker.TestThrottledAsyncChecker
            hadoop.hdfs.web.TestWebHdfsTimeouts



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11170
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12859725/HDFS-11170-05.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux aaca256ac6e0 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 3b908f7
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18784/artifact/patchprocess/diff-checkstyle-root.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18784/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18784/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18784/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/18784/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 22s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. 0 mvndep 1m 50s Maven dependency ordering for branch +1 mvninstall 11m 55s trunk passed +1 compile 19m 54s trunk passed +1 checkstyle 1m 52s trunk passed +1 mvnsite 2m 36s trunk passed +1 mvneclipse 1m 4s trunk passed +1 findbugs 4m 31s trunk passed +1 javadoc 2m 8s trunk passed 0 mvndep 0m 13s Maven dependency ordering for patch +1 mvninstall 1m 44s the patch passed +1 compile 15m 2s the patch passed +1 javac 15m 2s the patch passed -0 checkstyle 1m 52s root: The patch generated 4 new + 253 unchanged - 1 fixed = 257 total (was 254) +1 mvnsite 2m 31s the patch passed +1 mvneclipse 1m 3s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 4m 54s the patch passed +1 javadoc 2m 6s the patch passed -1 unit 7m 1s hadoop-common in the patch failed. +1 unit 1m 5s hadoop-hdfs-client in the patch passed. -1 unit 62m 39s hadoop-hdfs in the patch failed. +1 asflicense 0m 37s The patch does not generate ASF License warnings. 148m 30s Reason Tests Failed junit tests hadoop.security.TestShellBasedUnixGroupsMapping   hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure   hadoop.hdfs.server.datanode.checker.TestThrottledAsyncChecker   hadoop.hdfs.web.TestWebHdfsTimeouts Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11170 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12859725/HDFS-11170-05.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux aaca256ac6e0 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 3b908f7 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18784/artifact/patchprocess/diff-checkstyle-root.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/18784/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/18784/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18784/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/18784/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          Sammi SammiChen added a comment -

          Agree. Directly return FSDataOutputStream will definitly save steps. Uploaded a new patch to reflect the idea.

          Show
          Sammi SammiChen added a comment - Agree. Directly return FSDataOutputStream will definitly save steps. Uploaded a new patch to reflect the idea.
          Hide
          andrew.wang Andrew Wang added a comment -

          Hi Sammi, thanks for the rev,

          Inspired by Stack's comment, I played around with the patch. I think I got to a good spot where with methods/classes like the following:

          FileSystem.java
            public FSDataOutputStreamBuilder newFSDataOutputStreamBuilder(Path p) {
              return new FSDataOutputStreamBuilder(this, p);
            }
          
          DistributedFileSystem.java
            public static class HdfsDataOutputStreamBuilder
                extends FSDataOutputStreamBuilder {
          
              private final DistributedFileSystem dfs;
              private InetSocketAddress[] favoredNodes = null;
          
              public HdfsDataOutputStreamBuilder(
                  DistributedFileSystem dfs, Path path) {
                super(dfs, path);
                this.dfs = dfs;
              }
          
              public InetSocketAddress[] getFavoredNodes() {
                return favoredNodes == null ? null : favoredNodes.clone();
              }
          
              public HdfsDataOutputStreamBuilder setFavoredNodes(
                  InetSocketAddress[] nodes) {
                favoredNodes = nodes == null ? null : nodes.clone();
                return this;
              }
          
              @Override
              public HdfsDataOutputStream build() throws IOException {
                return dfs.create(getPath(), getPermission(),
                    getFlags(), getBufferSize(),
                    getReplication(), getBlockSize(), getProgress(), getChecksumOpt(),
                    getFavoredNodes());
              }
            }
          
            @Override
            public HdfsDataOutputStreamBuilder newFSDataOutputStreamBuilder(Path path) {
              return new HdfsDataOutputStreamBuilder(this, path);
            }
          

          Then there's a base class FSDataOutputStream that has public setters and protected getters. I also inlined the default behaviors for the getters for use by the subclasses, e.g.

            protected int getBufferSize() {
              if (bufferSize == null) {
                return fs.getConf().getInt(IO_FILE_BUFFER_SIZE_KEY,
                    IO_FILE_BUFFER_SIZE_DEFAULT);
              }
              return bufferSize;
            }
          

          Do you think this works? It saves some steps for the user and we don't need a separate Parameters object.

          Show
          andrew.wang Andrew Wang added a comment - Hi Sammi, thanks for the rev, Inspired by Stack's comment, I played around with the patch. I think I got to a good spot where with methods/classes like the following: FileSystem.java public FSDataOutputStreamBuilder newFSDataOutputStreamBuilder(Path p) { return new FSDataOutputStreamBuilder( this , p); } DistributedFileSystem.java public static class HdfsDataOutputStreamBuilder extends FSDataOutputStreamBuilder { private final DistributedFileSystem dfs; private InetSocketAddress[] favoredNodes = null ; public HdfsDataOutputStreamBuilder( DistributedFileSystem dfs, Path path) { super (dfs, path); this .dfs = dfs; } public InetSocketAddress[] getFavoredNodes() { return favoredNodes == null ? null : favoredNodes.clone(); } public HdfsDataOutputStreamBuilder setFavoredNodes( InetSocketAddress[] nodes) { favoredNodes = nodes == null ? null : nodes.clone(); return this ; } @Override public HdfsDataOutputStream build() throws IOException { return dfs.create(getPath(), getPermission(), getFlags(), getBufferSize(), getReplication(), getBlockSize(), getProgress(), getChecksumOpt(), getFavoredNodes()); } } @Override public HdfsDataOutputStreamBuilder newFSDataOutputStreamBuilder(Path path) { return new HdfsDataOutputStreamBuilder( this , path); } Then there's a base class FSDataOutputStream that has public setters and protected getters. I also inlined the default behaviors for the getters for use by the subclasses, e.g. protected int getBufferSize() { if (bufferSize == null ) { return fs.getConf().getInt(IO_FILE_BUFFER_SIZE_KEY, IO_FILE_BUFFER_SIZE_DEFAULT); } return bufferSize; } Do you think this works? It saves some steps for the user and we don't need a separate Parameters object.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 4 new or modified test files.
          0 mvndep 0m 13s Maven dependency ordering for branch
          +1 mvninstall 13m 3s trunk passed
          +1 compile 11m 52s trunk passed
          +1 checkstyle 2m 6s trunk passed
          +1 mvnsite 3m 5s trunk passed
          +1 mvneclipse 1m 8s trunk passed
          +1 findbugs 5m 25s trunk passed
          +1 javadoc 2m 25s trunk passed
          0 mvndep 0m 15s Maven dependency ordering for patch
          +1 mvninstall 2m 9s the patch passed
          +1 compile 11m 36s the patch passed
          +1 javac 11m 36s the patch passed
          -0 checkstyle 2m 5s root: The patch generated 6 new + 416 unchanged - 1 fixed = 422 total (was 417)
          +1 mvnsite 2m 57s the patch passed
          +1 mvneclipse 1m 9s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 5m 45s the patch passed
          +1 javadoc 2m 15s the patch passed
          -1 unit 8m 30s hadoop-common in the patch failed.
          +1 unit 1m 25s hadoop-hdfs-client in the patch passed.
          -1 unit 95m 44s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 56s The patch does not generate ASF License warnings.
          175m 56s



          Reason Tests
          Failed junit tests hadoop.security.TestKDiag
            hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure
            hadoop.hdfs.server.datanode.TestDataNodeUUID
            hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting
          Timed out junit tests org.apache.hadoop.hdfs.server.blockmanagement.TestBlockStatsMXBean



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11170
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12859551/HDFS-11170-04.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 270d6c1ab872 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 34a931c
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18769/artifact/patchprocess/diff-checkstyle-root.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18769/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18769/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18769/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/18769/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. +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 13s Maven dependency ordering for branch +1 mvninstall 13m 3s trunk passed +1 compile 11m 52s trunk passed +1 checkstyle 2m 6s trunk passed +1 mvnsite 3m 5s trunk passed +1 mvneclipse 1m 8s trunk passed +1 findbugs 5m 25s trunk passed +1 javadoc 2m 25s trunk passed 0 mvndep 0m 15s Maven dependency ordering for patch +1 mvninstall 2m 9s the patch passed +1 compile 11m 36s the patch passed +1 javac 11m 36s the patch passed -0 checkstyle 2m 5s root: The patch generated 6 new + 416 unchanged - 1 fixed = 422 total (was 417) +1 mvnsite 2m 57s the patch passed +1 mvneclipse 1m 9s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 5m 45s the patch passed +1 javadoc 2m 15s the patch passed -1 unit 8m 30s hadoop-common in the patch failed. +1 unit 1m 25s hadoop-hdfs-client in the patch passed. -1 unit 95m 44s hadoop-hdfs in the patch failed. +1 asflicense 0m 56s The patch does not generate ASF License warnings. 175m 56s Reason Tests Failed junit tests hadoop.security.TestKDiag   hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure   hadoop.hdfs.server.datanode.TestDataNodeUUID   hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting Timed out junit tests org.apache.hadoop.hdfs.server.blockmanagement.TestBlockStatsMXBean Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11170 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12859551/HDFS-11170-04.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 270d6c1ab872 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 34a931c Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18769/artifact/patchprocess/diff-checkstyle-root.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/18769/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/18769/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18769/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/18769/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          Sammi SammiChen added a comment -

          Hi Andrew Wang, do you mind take a look at the new uploaded patch? One thing I'm not for sure is the way current implementation of FileCreateParametersBuilder and FileCreateParameters. It may be not the classic builder pattern. But I think current implementation is more concise. I would like hear your opinions.

          Show
          Sammi SammiChen added a comment - Hi Andrew Wang , do you mind take a look at the new uploaded patch? One thing I'm not for sure is the way current implementation of FileCreateParametersBuilder and FileCreateParameters . It may be not the classic builder pattern. But I think current implementation is more concise. I would like hear your opinions.
          Hide
          Sammi SammiChen added a comment -

          Thanks Andrew Wang, Xiaobing Zhou and stack for your suggestions! I will soon upload a new patch to address all the concerns.

          Show
          Sammi SammiChen added a comment - Thanks Andrew Wang , Xiaobing Zhou and stack for your suggestions! I will soon upload a new patch to address all the concerns.
          Hide
          stack stack added a comment -

          I took a look at 03

          • CreateBuilder.java needs license.
          • I was going to suggest that CreateBuilder is too generic a name but it seems like we are misusing builder and that is why the confusion. For example:

          1427 DistributedFileSystemCreateBuilder builder =
          1428 fs.newCreateBuilder(testFilePath).build();
          1429 FSDataOutputStream out = fs.create(builder);

          i.e. I build a 'create' builder to pass to a create function that then 'builds' the wanted object.

          I'd think that when I called build on the builder, that I'd get back a FSDataOutputStream – not a 'Builder' (this is what Xiaobing Zhou says above now I've read those comments).

          Show
          stack stack added a comment - I took a look at 03 CreateBuilder.java needs license. I was going to suggest that CreateBuilder is too generic a name but it seems like we are misusing builder and that is why the confusion. For example: 1427 DistributedFileSystemCreateBuilder builder = 1428 fs.newCreateBuilder(testFilePath).build(); 1429 FSDataOutputStream out = fs.create(builder); i.e. I build a 'create' builder to pass to a create function that then 'builds' the wanted object. I'd think that when I called build on the builder, that I'd get back a FSDataOutputStream – not a 'Builder' (this is what Xiaobing Zhou says above now I've read those comments).
          Hide
          andrew.wang Andrew Wang added a comment -

          Can we change the name (i.e. CreateBuilder) to be more self-descriptive, e.g. PathCreateBuilder or sth like that?

          Actually, how about we combine this with my above review comment, and have a CreateParametersBuilder that returns a CreateParameters? I like the symmetry.

          Show
          andrew.wang Andrew Wang added a comment - Can we change the name (i.e. CreateBuilder) to be more self-descriptive, e.g. PathCreateBuilder or sth like that? Actually, how about we combine this with my above review comment, and have a CreateParametersBuilder that returns a CreateParameters ? I like the symmetry.
          Hide
          xiaobingo Xiaobing Zhou added a comment -

          Thanks Wei Zhou for the work. Some comments:

          1. Can we change the name (i.e. CreateBuilder) to be more self-descriptive, e.g. PathCreateBuilder or sth like that?
          2. XXXBuilder#build usually returns the something being built, XXX for example, CreateBuilder#build gives out CreateBuilder itself, which is kind of mind mismatch.
          3. Can you add comments to the public method FileSystem#create?
          Show
          xiaobingo Xiaobing Zhou added a comment - Thanks Wei Zhou for the work. Some comments: Can we change the name (i.e. CreateBuilder) to be more self-descriptive, e.g. PathCreateBuilder or sth like that? XXXBuilder#build usually returns the something being built, XXX for example, CreateBuilder#build gives out CreateBuilder itself, which is kind of mind mismatch. Can you add comments to the public method FileSystem#create?
          Hide
          andrew.wang Andrew Wang added a comment -

          Hi Sammi, thanks for taking this over, a few review comments:

          General:

          • Can we make the new DFS create method private rather than public?
          • Canonically, the builder pattern's build pattern normally returns an immutable argument with just getters. Maybe call this object CreateParameters?

          Error checking in build:

          We should also try to do fill in defaults similar to how FileSystem currently works. For example:

          • build fills in some defaults like OVERWRITE and the replication and block size, but when I trace through the create overloads, it doesn't go all the way. We're missing the default permission and umask. Could you check for anything else?
          • We also need to be careful to differentiate between a set 0 and the value not being set for things like replication and block size. I think we can do this by internally using the nullable numeric types like Long and Short.
          • The builder API setters though should still use the primitive types like normal create calls. Need to also fix setReplication which takes a Short rather than a short currently.

          stack do you mind taking a quick look at the API? There are some examples in the test cases. I'm hoping this can help clean up the HBase-side code that does favored nodes.

          Show
          andrew.wang Andrew Wang added a comment - Hi Sammi, thanks for taking this over, a few review comments: General: Can we make the new DFS create method private rather than public? Canonically, the builder pattern's build pattern normally returns an immutable argument with just getters. Maybe call this object CreateParameters ? Error checking in build : We should also try to do fill in defaults similar to how FileSystem currently works. For example: build fills in some defaults like OVERWRITE and the replication and block size, but when I trace through the create overloads, it doesn't go all the way. We're missing the default permission and umask. Could you check for anything else? We also need to be careful to differentiate between a set 0 and the value not being set for things like replication and block size. I think we can do this by internally using the nullable numeric types like Long and Short . The builder API setters though should still use the primitive types like normal create calls. Need to also fix setReplication which takes a Short rather than a short currently. stack do you mind taking a quick look at the API? There are some examples in the test cases. I'm hoping this can help clean up the HBase-side code that does favored nodes.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 14s Docker mode activated.
          +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 1m 55s Maven dependency ordering for branch
          +1 mvninstall 12m 27s trunk passed
          -1 compile 6m 50s root in trunk failed.
          +1 checkstyle 2m 2s trunk passed
          +1 mvnsite 2m 53s trunk passed
          +1 mvneclipse 1m 13s trunk passed
          +1 findbugs 5m 3s trunk passed
          +1 javadoc 2m 18s trunk passed
          0 mvndep 0m 14s Maven dependency ordering for patch
          +1 mvninstall 1m 53s the patch passed
          -1 compile 4m 50s root in the patch failed.
          -1 javac 4m 50s root in the patch failed.
          -0 checkstyle 1m 51s root: The patch generated 3 new + 416 unchanged - 1 fixed = 419 total (was 417)
          +1 mvnsite 2m 15s the patch passed
          +1 mvneclipse 0m 37s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 4m 51s the patch passed
          +1 javadoc 1m 40s the patch passed
          +1 unit 8m 36s hadoop-common in the patch passed.
          +1 unit 0m 55s hadoop-hdfs-client in the patch passed.
          -1 unit 66m 25s hadoop-hdfs in the patch failed.
          -1 asflicense 0m 24s The patch generated 1 ASF License warnings.
          130m 37s



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



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11170
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12858819/HDFS-11170-03.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux c0c88d83a902 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / cc1292e
          Default Java 1.8.0_121
          compile https://builds.apache.org/job/PreCommit-HDFS-Build/18725/artifact/patchprocess/branch-compile-root.txt
          findbugs v3.0.0
          compile https://builds.apache.org/job/PreCommit-HDFS-Build/18725/artifact/patchprocess/patch-compile-root.txt
          javac https://builds.apache.org/job/PreCommit-HDFS-Build/18725/artifact/patchprocess/patch-compile-root.txt
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18725/artifact/patchprocess/diff-checkstyle-root.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18725/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18725/testReport/
          asflicense https://builds.apache.org/job/PreCommit-HDFS-Build/18725/artifact/patchprocess/patch-asflicense-problems.txt
          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/18725/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 14s Docker mode activated. +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 1m 55s Maven dependency ordering for branch +1 mvninstall 12m 27s trunk passed -1 compile 6m 50s root in trunk failed. +1 checkstyle 2m 2s trunk passed +1 mvnsite 2m 53s trunk passed +1 mvneclipse 1m 13s trunk passed +1 findbugs 5m 3s trunk passed +1 javadoc 2m 18s trunk passed 0 mvndep 0m 14s Maven dependency ordering for patch +1 mvninstall 1m 53s the patch passed -1 compile 4m 50s root in the patch failed. -1 javac 4m 50s root in the patch failed. -0 checkstyle 1m 51s root: The patch generated 3 new + 416 unchanged - 1 fixed = 419 total (was 417) +1 mvnsite 2m 15s the patch passed +1 mvneclipse 0m 37s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 4m 51s the patch passed +1 javadoc 1m 40s the patch passed +1 unit 8m 36s hadoop-common in the patch passed. +1 unit 0m 55s hadoop-hdfs-client in the patch passed. -1 unit 66m 25s hadoop-hdfs in the patch failed. -1 asflicense 0m 24s The patch generated 1 ASF License warnings. 130m 37s Reason Tests Failed junit tests hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting   hadoop.hdfs.server.datanode.fsdataset.impl.TestLazyPersistReplicaRecovery Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11170 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12858819/HDFS-11170-03.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux c0c88d83a902 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / cc1292e Default Java 1.8.0_121 compile https://builds.apache.org/job/PreCommit-HDFS-Build/18725/artifact/patchprocess/branch-compile-root.txt findbugs v3.0.0 compile https://builds.apache.org/job/PreCommit-HDFS-Build/18725/artifact/patchprocess/patch-compile-root.txt javac https://builds.apache.org/job/PreCommit-HDFS-Build/18725/artifact/patchprocess/patch-compile-root.txt checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18725/artifact/patchprocess/diff-checkstyle-root.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/18725/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18725/testReport/ asflicense https://builds.apache.org/job/PreCommit-HDFS-Build/18725/artifact/patchprocess/patch-asflicense-problems.txt 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/18725/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          Sammi SammiChen added a comment -

          A new patch uploaded. Javadoc issue and two failed unit test TestHarFileSystem and TestFilterFileSystem are addressed. All other failed unit tests and reported checkstyle issued are double checked. They are all not relevant.

          Show
          Sammi SammiChen added a comment - A new patch uploaded. Javadoc issue and two failed unit test TestHarFileSystem and TestFilterFileSystem are addressed. All other failed unit tests and reported checkstyle issued are double checked. They are all not relevant.
          Hide
          andrew.wang Andrew Wang added a comment -

          Great, thanks SammiChen and Wei Zhou! I feel like we've already put in good work on this JIRA, so it would be great to get it committed.

          Show
          andrew.wang Andrew Wang added a comment - Great, thanks SammiChen and Wei Zhou ! I feel like we've already put in good work on this JIRA, so it would be great to get it committed.
          Hide
          zhouwei Wei Zhou added a comment -

          Thanks Andrew Wang for the reminder and sorry for the delay due to some other issues! Thanks SammiChen for taking over this JIRA!

          Show
          zhouwei Wei Zhou added a comment - Thanks Andrew Wang for the reminder and sorry for the delay due to some other issues! Thanks SammiChen for taking over this JIRA!
          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 4 new or modified test files.
          0 mvndep 0m 15s Maven dependency ordering for branch
          +1 mvninstall 12m 22s trunk passed
          -1 compile 6m 56s root in trunk failed.
          +1 checkstyle 2m 2s trunk passed
          +1 mvnsite 2m 53s trunk passed
          +1 mvneclipse 1m 12s trunk passed
          +1 findbugs 5m 1s trunk passed
          +1 javadoc 2m 16s trunk passed
          0 mvndep 0m 14s Maven dependency ordering for patch
          +1 mvninstall 1m 52s the patch passed
          -1 compile 6m 20s root in the patch failed.
          -1 javac 6m 21s root in the patch failed.
          -0 checkstyle 2m 2s root: The patch generated 3 new + 333 unchanged - 1 fixed = 336 total (was 334)
          +1 mvnsite 2m 48s the patch passed
          +1 mvneclipse 1m 9s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 5m 23s the patch passed
          -1 javadoc 0m 53s hadoop-common-project_hadoop-common generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
          -1 unit 8m 15s hadoop-common in the patch failed.
          +1 unit 1m 6s hadoop-hdfs-client in the patch passed.
          -1 unit 64m 25s hadoop-hdfs in the patch failed.
          -1 asflicense 0m 43s The patch generated 1 ASF License warnings.
          131m 26s



          Reason Tests
          Failed junit tests hadoop.fs.TestHarFileSystem
            hadoop.fs.TestFilterFileSystem
            hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting
            hadoop.hdfs.server.namenode.ha.TestHASafeMode
            hadoop.hdfs.web.TestWebHdfsFileSystemContract
            hadoop.hdfs.TestHDFSFileSystemContract



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11170
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12858628/HDFS-11170-02.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux e56beeef34a3 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / e5f2eed
          Default Java 1.8.0_121
          compile https://builds.apache.org/job/PreCommit-HDFS-Build/18707/artifact/patchprocess/branch-compile-root.txt
          findbugs v3.0.0
          compile https://builds.apache.org/job/PreCommit-HDFS-Build/18707/artifact/patchprocess/patch-compile-root.txt
          javac https://builds.apache.org/job/PreCommit-HDFS-Build/18707/artifact/patchprocess/patch-compile-root.txt
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18707/artifact/patchprocess/diff-checkstyle-root.txt
          javadoc https://builds.apache.org/job/PreCommit-HDFS-Build/18707/artifact/patchprocess/diff-javadoc-javadoc-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18707/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18707/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18707/testReport/
          asflicense https://builds.apache.org/job/PreCommit-HDFS-Build/18707/artifact/patchprocess/patch-asflicense-problems.txt
          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/18707/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 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 4 new or modified test files. 0 mvndep 0m 15s Maven dependency ordering for branch +1 mvninstall 12m 22s trunk passed -1 compile 6m 56s root in trunk failed. +1 checkstyle 2m 2s trunk passed +1 mvnsite 2m 53s trunk passed +1 mvneclipse 1m 12s trunk passed +1 findbugs 5m 1s trunk passed +1 javadoc 2m 16s trunk passed 0 mvndep 0m 14s Maven dependency ordering for patch +1 mvninstall 1m 52s the patch passed -1 compile 6m 20s root in the patch failed. -1 javac 6m 21s root in the patch failed. -0 checkstyle 2m 2s root: The patch generated 3 new + 333 unchanged - 1 fixed = 336 total (was 334) +1 mvnsite 2m 48s the patch passed +1 mvneclipse 1m 9s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 5m 23s the patch passed -1 javadoc 0m 53s hadoop-common-project_hadoop-common generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) -1 unit 8m 15s hadoop-common in the patch failed. +1 unit 1m 6s hadoop-hdfs-client in the patch passed. -1 unit 64m 25s hadoop-hdfs in the patch failed. -1 asflicense 0m 43s The patch generated 1 ASF License warnings. 131m 26s Reason Tests Failed junit tests hadoop.fs.TestHarFileSystem   hadoop.fs.TestFilterFileSystem   hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting   hadoop.hdfs.server.namenode.ha.TestHASafeMode   hadoop.hdfs.web.TestWebHdfsFileSystemContract   hadoop.hdfs.TestHDFSFileSystemContract Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11170 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12858628/HDFS-11170-02.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux e56beeef34a3 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / e5f2eed Default Java 1.8.0_121 compile https://builds.apache.org/job/PreCommit-HDFS-Build/18707/artifact/patchprocess/branch-compile-root.txt findbugs v3.0.0 compile https://builds.apache.org/job/PreCommit-HDFS-Build/18707/artifact/patchprocess/patch-compile-root.txt javac https://builds.apache.org/job/PreCommit-HDFS-Build/18707/artifact/patchprocess/patch-compile-root.txt checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18707/artifact/patchprocess/diff-checkstyle-root.txt javadoc https://builds.apache.org/job/PreCommit-HDFS-Build/18707/artifact/patchprocess/diff-javadoc-javadoc-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/18707/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/18707/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18707/testReport/ asflicense https://builds.apache.org/job/PreCommit-HDFS-Build/18707/artifact/patchprocess/patch-asflicense-problems.txt 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/18707/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          Sammi SammiChen added a comment -

          I will pick it up and continue the work.

          Show
          Sammi SammiChen added a comment - I will pick it up and continue the work.
          Hide
          andrew.wang Andrew Wang added a comment -

          Hi Wei Zhou, do you plan to continue to work on this patch? Would be great to get this in.

          Show
          andrew.wang Andrew Wang added a comment - Hi Wei Zhou , do you plan to continue to work on this patch? Would be great to get this in.
          Hide
          andrew.wang Andrew Wang added a comment -

          Hi Wei Zhou, thanks for revving, haven't looked at the patch yet but one question:

          If so, I found there are some difference in implementation between create(..., final InetSocketAddress[] favoredNodes) and create(..., final ChecksumOpt checksumOpt).

          Could you expand on these differences? This is all surprising from a user perspective, which is why I'm pushing for a single unified create call.

          If these differences are there for a good reason, we need to document and enforce mutually exclusive parameters in the builder's API so we aren't silently dropping parameters.

          Show
          andrew.wang Andrew Wang added a comment - Hi Wei Zhou , thanks for revving, haven't looked at the patch yet but one question: If so, I found there are some difference in implementation between create(..., final InetSocketAddress[] favoredNodes) and create(..., final ChecksumOpt checksumOpt). Could you expand on these differences? This is all surprising from a user perspective, which is why I'm pushing for a single unified create call. If these differences are there for a good reason, we need to document and enforce mutually exclusive parameters in the builder's API so we aren't silently dropping parameters.
          Hide
          zhouwei Wei Zhou added a comment -

          Hi Andrew Wang, thanks for reviewing the patch and the great suggestions!
          The patch is updated with some issues:

          a factory method in FileSystem to get the FS-specific builder?
          

          I added a function newCreateBuilder in FileSystem to generate specific file system create builder, I'm not sure it proper to do it in this way or not.

          add a new private DFS#create that takes checksumOpt and flags along with favoredNodes is set.
          

          Do you mean all the create functions in DistributedFileSystem goes down to the same private DFS#create? If so, I found there are some difference in implementation between create(..., final InetSocketAddress[] favoredNodes) and create(..., final ChecksumOpt checksumOpt).

          Thanks again!

          Show
          zhouwei Wei Zhou added a comment - Hi Andrew Wang , thanks for reviewing the patch and the great suggestions! The patch is updated with some issues: a factory method in FileSystem to get the FS-specific builder? I added a function newCreateBuilder in FileSystem to generate specific file system create builder, I'm not sure it proper to do it in this way or not. add a new private DFS#create that takes checksumOpt and flags along with favoredNodes is set. Do you mean all the create functions in DistributedFileSystem goes down to the same private DFS#create? If so, I found there are some difference in implementation between create(..., final InetSocketAddress[] favoredNodes) and create(..., final ChecksumOpt checksumOpt) . Thanks again!
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 13s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          0 mvndep 0m 16s Maven dependency ordering for branch
          +1 mvninstall 14m 22s trunk passed
          +1 compile 11m 30s trunk passed
          +1 checkstyle 1m 51s trunk passed
          +1 mvnsite 3m 2s trunk passed
          +1 mvneclipse 0m 59s trunk passed
          +1 findbugs 5m 32s trunk passed
          +1 javadoc 2m 18s trunk passed
          0 mvndep 0m 18s Maven dependency ordering for patch
          +1 mvninstall 2m 28s the patch passed
          +1 compile 11m 14s the patch passed
          +1 javac 11m 14s the patch passed
          -0 checkstyle 1m 42s root: The patch generated 12 new + 133 unchanged - 1 fixed = 145 total (was 134)
          +1 mvnsite 2m 49s the patch passed
          +1 mvneclipse 0m 59s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          -1 findbugs 1m 58s hadoop-hdfs-project/hadoop-hdfs-client generated 2 new + 0 unchanged - 0 fixed = 2 total (was 0)
          +1 javadoc 2m 9s the patch passed
          -1 unit 7m 56s hadoop-common in the patch failed.
          +1 unit 1m 8s hadoop-hdfs-client in the patch passed.
          -1 unit 69m 26s hadoop-hdfs in the patch failed.
          -1 asflicense 0m 38s The patch generated 1 ASF License warnings.
          148m 16s



          Reason Tests
          FindBugs module:hadoop-hdfs-project/hadoop-hdfs-client
            org.apache.hadoop.hdfs.DistributedFileSystem$DistributedFileSystemCreateBuilder.getFavoredNodes() may expose internal representation by returning DistributedFileSystem$DistributedFileSystemCreateBuilder.favoredNodes At DistributedFileSystem.java:by returning DistributedFileSystem$DistributedFileSystemCreateBuilder.favoredNodes At DistributedFileSystem.java:[line 2567]
            org.apache.hadoop.hdfs.DistributedFileSystem$DistributedFileSystemCreateBuilder.setFavoredNodes(InetSocketAddress[]) may expose internal representation by storing an externally mutable object into DistributedFileSystem$DistributedFileSystemCreateBuilder.favoredNodes At DistributedFileSystem.java:by storing an externally mutable object into DistributedFileSystem$DistributedFileSystemCreateBuilder.favoredNodes At DistributedFileSystem.java:[line 2572]
          Failed junit tests hadoop.fs.TestHarFileSystem
            hadoop.fs.TestFilterFileSystem
            hadoop.hdfs.server.namenode.TestNameNodeRespectsBindHostKeys



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11170
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12845377/HDFS-11170-00.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 853db8641334 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 / a0a2761
          Default Java 1.8.0_111
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18027/artifact/patchprocess/diff-checkstyle-root.txt
          findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/18027/artifact/patchprocess/new-findbugs-hadoop-hdfs-project_hadoop-hdfs-client.html
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18027/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18027/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18027/testReport/
          asflicense https://builds.apache.org/job/PreCommit-HDFS-Build/18027/artifact/patchprocess/patch-asflicense-problems.txt
          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/18027/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 13s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. 0 mvndep 0m 16s Maven dependency ordering for branch +1 mvninstall 14m 22s trunk passed +1 compile 11m 30s trunk passed +1 checkstyle 1m 51s trunk passed +1 mvnsite 3m 2s trunk passed +1 mvneclipse 0m 59s trunk passed +1 findbugs 5m 32s trunk passed +1 javadoc 2m 18s trunk passed 0 mvndep 0m 18s Maven dependency ordering for patch +1 mvninstall 2m 28s the patch passed +1 compile 11m 14s the patch passed +1 javac 11m 14s the patch passed -0 checkstyle 1m 42s root: The patch generated 12 new + 133 unchanged - 1 fixed = 145 total (was 134) +1 mvnsite 2m 49s the patch passed +1 mvneclipse 0m 59s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. -1 findbugs 1m 58s hadoop-hdfs-project/hadoop-hdfs-client generated 2 new + 0 unchanged - 0 fixed = 2 total (was 0) +1 javadoc 2m 9s the patch passed -1 unit 7m 56s hadoop-common in the patch failed. +1 unit 1m 8s hadoop-hdfs-client in the patch passed. -1 unit 69m 26s hadoop-hdfs in the patch failed. -1 asflicense 0m 38s The patch generated 1 ASF License warnings. 148m 16s Reason Tests FindBugs module:hadoop-hdfs-project/hadoop-hdfs-client   org.apache.hadoop.hdfs.DistributedFileSystem$DistributedFileSystemCreateBuilder.getFavoredNodes() may expose internal representation by returning DistributedFileSystem$DistributedFileSystemCreateBuilder.favoredNodes At DistributedFileSystem.java:by returning DistributedFileSystem$DistributedFileSystemCreateBuilder.favoredNodes At DistributedFileSystem.java: [line 2567]   org.apache.hadoop.hdfs.DistributedFileSystem$DistributedFileSystemCreateBuilder.setFavoredNodes(InetSocketAddress[]) may expose internal representation by storing an externally mutable object into DistributedFileSystem$DistributedFileSystemCreateBuilder.favoredNodes At DistributedFileSystem.java:by storing an externally mutable object into DistributedFileSystem$DistributedFileSystemCreateBuilder.favoredNodes At DistributedFileSystem.java: [line 2572] Failed junit tests hadoop.fs.TestHarFileSystem   hadoop.fs.TestFilterFileSystem   hadoop.hdfs.server.namenode.TestNameNodeRespectsBindHostKeys Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11170 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12845377/HDFS-11170-00.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 853db8641334 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 / a0a2761 Default Java 1.8.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18027/artifact/patchprocess/diff-checkstyle-root.txt findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/18027/artifact/patchprocess/new-findbugs-hadoop-hdfs-project_hadoop-hdfs-client.html unit https://builds.apache.org/job/PreCommit-HDFS-Build/18027/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/18027/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18027/testReport/ asflicense https://builds.apache.org/job/PreCommit-HDFS-Build/18027/artifact/patchprocess/patch-asflicense-problems.txt 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/18027/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 also retriggered the precommit build manually, somehow it didn't run before.

          Show
          andrew.wang Andrew Wang added a comment - I also retriggered the precommit build manually, somehow it didn't run before.
          Hide
          andrew.wang Andrew Wang added a comment -

          Thanks for working on this Wei Zhou, looks great overall A few review comments:

          • Since path is a required parameter, it should be passed as an argument to the CreateBuilder constructor. This way we don't need to check it later.
          • It looks like overwrite is sort of deprecated in favor of flags, so I think we should only support flags.
          • What do you think about having a factory method in FileSystem to get the FS-specific builder? It's nice since then users can have generic code for the common CreateBuilder args, then test if it's a DFSCreateBuilder to conditionally set additional parameters. This way we don't need to pass in the FileSystem as a CreateBuilder argument too, it can set the correct defaults in the constructor.
          • Would prefer we don't change the flags semantics to take null, the builder can set an empty EnumSet as appropriate.
          • In FileSystem#doCreate and DFS#doCreate, I'd prefer we always call the same create method with the defaults filled in appropriately. I think this works if we stop supporting overwrite, and add a new private DFS#create that takes checksumOpt and flags along with favoredNodes is set.
          Show
          andrew.wang Andrew Wang added a comment - Thanks for working on this Wei Zhou , looks great overall A few review comments: Since path is a required parameter, it should be passed as an argument to the CreateBuilder constructor. This way we don't need to check it later. It looks like overwrite is sort of deprecated in favor of flags , so I think we should only support flags . What do you think about having a factory method in FileSystem to get the FS-specific builder? It's nice since then users can have generic code for the common CreateBuilder args, then test if it's a DFSCreateBuilder to conditionally set additional parameters. This way we don't need to pass in the FileSystem as a CreateBuilder argument too, it can set the correct defaults in the constructor. Would prefer we don't change the flags semantics to take null, the builder can set an empty EnumSet as appropriate. In FileSystem#doCreate and DFS#doCreate, I'd prefer we always call the same create method with the defaults filled in appropriately. I think this works if we stop supporting overwrite , and add a new private DFS#create that takes checksumOpt and flags along with favoredNodes is set.
          Hide
          zhouwei Wei Zhou added a comment -

          This initial patch. Thanks SammiChen for the great help!

          Show
          zhouwei Wei Zhou added a comment - This initial patch. Thanks SammiChen for the great help!
          Hide
          Sammi SammiChen added a comment -

          THanks Andrew Wang for provide such good suggestion! Will follow the throughts .

          Show
          Sammi SammiChen added a comment - THanks Andrew Wang for provide such good suggestion! Will follow the throughts .
          Hide
          andrew.wang Andrew Wang added a comment -

          Thinking about it a little more, I think we just need a factory method that returns a CreateBuilder in FileSystem. Filesystem subclasses can override and extend as desired. Users can query for additional capabilities by downcasting the returned Builder. If we want to be more generic, we can add interfaces for adding favored nodes and erasure coding parameters that Builder subclasses can implement. This is similar to how the different FSDataOutputStreams implement Syncable.

          Show
          andrew.wang Andrew Wang added a comment - Thinking about it a little more, I think we just need a factory method that returns a CreateBuilder in FileSystem. Filesystem subclasses can override and extend as desired. Users can query for additional capabilities by downcasting the returned Builder. If we want to be more generic, we can add interfaces for adding favored nodes and erasure coding parameters that Builder subclasses can implement. This is similar to how the different FSDataOutputStreams implement Syncable.
          Hide
          andrew.wang Andrew Wang added a comment -

          Thanks for the investigation Sammi. Do you think we can do something with subclasses? i.e. FileSystem has a CreateBuilder and HdfsAdmin has a HdfsCreateBuilder that extends CreateBuilder. We build up the call, then call invokes the appropriate FileSystem-specific create method.

          Show
          andrew.wang Andrew Wang added a comment - Thanks for the investigation Sammi. Do you think we can do something with subclasses? i.e. FileSystem has a CreateBuilder and HdfsAdmin has a HdfsCreateBuilder that extends CreateBuilder. We build up the call, then call invokes the appropriate FileSystem-specific create method.
          Hide
          Sammi SammiChen added a comment -

          Hi Andrew Wang, as we have disuccsed in HDFS-10996, our initial goal of this task is to make it easy for user to pass parameter to create function, and also make it easy for feature developer to introduce more new parameters. After some investigation, I find ErasureCodingPolicy parameter type is only restricted in DistributedFileSystem scope. It can't go into filesystem. Other than ErasureCodingPolicy parameter, InetSocketAddress[] favoredNodes create parameter is also unique to DistributedFileSystem. I guess for other file system class, such as S3, or future cloud file system, it may also has its own specific create parameters. My point here is it seems there is no way to have a general create builder including both create parameters in FileSystem class and parameters in {{DistributedFileSystem}}class. There has to be a create builder for each class, which I think is not an elegant design. My prefer is we only use builder for the newly added create function in HDFS-10996. Any suggestion?

          Show
          Sammi SammiChen added a comment - Hi Andrew Wang , as we have disuccsed in HDFS-10996 , our initial goal of this task is to make it easy for user to pass parameter to create function, and also make it easy for feature developer to introduce more new parameters. After some investigation, I find ErasureCodingPolicy parameter type is only restricted in DistributedFileSystem scope. It can't go into filesystem . Other than ErasureCodingPolicy parameter, InetSocketAddress[] favoredNodes create parameter is also unique to DistributedFileSystem . I guess for other file system class, such as S3, or future cloud file system, it may also has its own specific create parameters. My point here is it seems there is no way to have a general create builder including both create parameters in FileSystem class and parameters in {{DistributedFileSystem}}class. There has to be a create builder for each class, which I think is not an elegant design. My prefer is we only use builder for the newly added create function in HDFS-10996 . Any suggestion?

            People

            • Assignee:
              Sammi SammiChen
              Reporter:
              Sammi SammiChen
            • Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development