Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-3319

DFSOutputStream should not start a thread in constructors

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0.0-alpha
    • Component/s: hdfs-client
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      DFSOutputStream starts the DataStreamer thread in its constructors. This is a known bad programming practice. It will generate findbugs warnings if the class is not final.

      1. h3319_20120424.patch
        6 kB
        Tsz Wo Nicholas Sze
      2. h3319_20120425.patch
        7 kB
        Tsz Wo Nicholas Sze

        Issue Links

          Activity

          Tsz Wo Nicholas Sze created issue -
          Tsz Wo Nicholas Sze made changes -
          Field Original Value New Value
          Component/s hdfs client [ 12312928 ]
          Hide
          Tsz Wo Nicholas Sze added a comment -

          h3319_20120424.patch: add static methods.

          Show
          Tsz Wo Nicholas Sze added a comment - h3319_20120424.patch: add static methods.
          Tsz Wo Nicholas Sze made changes -
          Attachment h3319_20120424.patch [ 12524090 ]
          Tsz Wo Nicholas Sze made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Hadoop QA added a comment -

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

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

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

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.fs.TestUrlStreamHandler

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2326//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2326//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12524090/h3319_20120424.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified test files. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.fs.TestUrlStreamHandler +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2326//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2326//console This message is automatically generated.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          The failure of TestUrlStreamHandler is not related to this.

          Show
          Tsz Wo Nicholas Sze added a comment - The failure of TestUrlStreamHandler is not related to this.
          Tsz Wo Nicholas Sze made changes -
          Link This issue relates to HDFS-3298 [ HDFS-3298 ]
          Hide
          Uma Maheswara Rao G added a comment -

          Hi Nicholas, I just reviewed the patch.

          Patch looks nice. One minor comment:

          /**
              * Create a new output stream to the given DataNode.
              * @see ClientProtocol#create(String, FsPermission, String, boolean, short, long)
              */
          -  DFSOutputStream(DFSClient dfsClient, String src, int buffersize, Progressable progress,
          +  private DFSOutputStream(DFSClient dfsClient, String src, int buffersize, Progressable progress,
                 LocatedBlock lastBlock, HdfsFileStatus stat,
                 DataChecksum checksum) throws IOException {
          

          This particular ctor we used for append...but doc says create to see create.
          Actually this particular Javadoc not added by this patch. Since we got the room here to change, we can update as part o it.

          After addressing this comment, I am +1 for the patch.

          Show
          Uma Maheswara Rao G added a comment - Hi Nicholas, I just reviewed the patch. Patch looks nice. One minor comment: /** * Create a new output stream to the given DataNode. * @see ClientProtocol#create( String , FsPermission, String , boolean , short , long ) */ - DFSOutputStream(DFSClient dfsClient, String src, int buffersize, Progressable progress, + private DFSOutputStream(DFSClient dfsClient, String src, int buffersize, Progressable progress, LocatedBlock lastBlock, HdfsFileStatus stat, DataChecksum checksum) throws IOException { This particular ctor we used for append...but doc says create to see create. Actually this particular Javadoc not added by this patch. Since we got the room here to change, we can update as part o it. After addressing this comment, I am +1 for the patch.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          h3319_20120425.patch: updates javadoc.

          Show
          Tsz Wo Nicholas Sze added a comment - h3319_20120425.patch: updates javadoc.
          Tsz Wo Nicholas Sze made changes -
          Attachment h3319_20120425.patch [ 12524334 ]
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Uma, thanks for the review.

          I have committed this.

          Show
          Tsz Wo Nicholas Sze added a comment - Uma, thanks for the review. I have committed this.
          Tsz Wo Nicholas Sze made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags Reviewed [ 10343 ]
          Fix Version/s 2.0.0 [ 12320353 ]
          Resolution Fixed [ 1 ]
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #2206 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2206/)
          HDFS-3319. Change DFSOutputStream to not to start a thread in constructors. (Revision 1330535)

          Result = SUCCESS
          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1330535
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRenewer.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #2206 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2206/ ) HDFS-3319 . Change DFSOutputStream to not to start a thread in constructors. (Revision 1330535) Result = SUCCESS szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1330535 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRenewer.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #2132 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2132/)
          HDFS-3319. Change DFSOutputStream to not to start a thread in constructors. (Revision 1330535)

          Result = SUCCESS
          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1330535
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRenewer.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #2132 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2132/ ) HDFS-3319 . Change DFSOutputStream to not to start a thread in constructors. (Revision 1330535) Result = SUCCESS szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1330535 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRenewer.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #2149 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2149/)
          HDFS-3319. Change DFSOutputStream to not to start a thread in constructors. (Revision 1330535)

          Result = SUCCESS
          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1330535
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRenewer.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #2149 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2149/ ) HDFS-3319 . Change DFSOutputStream to not to start a thread in constructors. (Revision 1330535) Result = SUCCESS szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1330535 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRenewer.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1026 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1026/)
          HDFS-3319. Change DFSOutputStream to not to start a thread in constructors. (Revision 1330535)

          Result = FAILURE
          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1330535
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRenewer.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1026 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1026/ ) HDFS-3319 . Change DFSOutputStream to not to start a thread in constructors. (Revision 1330535) Result = FAILURE szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1330535 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRenewer.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1061 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1061/)
          HDFS-3319. Change DFSOutputStream to not to start a thread in constructors. (Revision 1330535)

          Result = FAILURE
          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1330535
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRenewer.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1061 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1061/ ) HDFS-3319 . Change DFSOutputStream to not to start a thread in constructors. (Revision 1330535) Result = FAILURE szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1330535 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSClient.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRenewer.java

            People

            • Assignee:
              Tsz Wo Nicholas Sze
              Reporter:
              Tsz Wo Nicholas Sze
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development