Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.20.1, 0.21.0
    • Fix Version/s: 0.20.1, 0.21.0
    • Component/s: hdfs-client
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      There are 5 constructors in DFSClient. It seems unnecessary.

      1. h527_20090804.patch
        8 kB
        Tsz Wo Nicholas Sze
      2. h527_20090804b.patch
        8 kB
        Tsz Wo Nicholas Sze
      3. h527_20090804c.patch
        9 kB
        Tsz Wo Nicholas Sze
      4. h527_20090804c_0.20.patch
        8 kB
        Tsz Wo Nicholas Sze

        Issue Links

          Activity

          Hide
          Tsz Wo Nicholas Sze added a comment -

          h527_20090804.patch:

          • Removed DFSClient(Configuration conf). It is not very useful. Only NamenodeFsck and a few tests use it.
          • Changed namenode, rpcNamenode to final.
          • Changed DFSClient(InetSocketAddress nameNodeAddr, Configuration conf, FileSystem.Statistics stats) to package private.
          • Combined the remaining constructors.
          Show
          Tsz Wo Nicholas Sze added a comment - h527_20090804.patch: Removed DFSClient(Configuration conf). It is not very useful. Only NamenodeFsck and a few tests use it. Changed namenode, rpcNamenode to final. Changed DFSClient(InetSocketAddress nameNodeAddr, Configuration conf, FileSystem.Statistics stats) to package private. Combined the remaining constructors.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > Removed DFSClient(Configuration conf). It is not very useful. Only NamenodeFsck and a few tests use it.
          Oops, we should deprecate it first.

          > Changed DFSClient(InetSocketAddress nameNodeAddr, Configuration conf, FileSystem.Statistics stats) to package private.
          We cannot change it to package private since it is a public API.

          h527_20090804b.patch:

          • deprecated DFSClient(Configuration conf)
          • kept DFSClient(InetSocketAddress nameNodeAddr, Configuration conf, FileSystem.Statistics stats) public.

          Then, this is no longer an incompatible change. Note that removing public DFSClient(ClientProtocol namenode, ClientProtocol rpcNamenode, Configuration conf, FileSystem.Statistics stats) is fine since it is not yet released.

          Show
          Tsz Wo Nicholas Sze added a comment - > Removed DFSClient(Configuration conf). It is not very useful. Only NamenodeFsck and a few tests use it. Oops, we should deprecate it first. > Changed DFSClient(InetSocketAddress nameNodeAddr, Configuration conf, FileSystem.Statistics stats) to package private. We cannot change it to package private since it is a public API. h527_20090804b.patch: deprecated DFSClient(Configuration conf) kept DFSClient(InetSocketAddress nameNodeAddr, Configuration conf, FileSystem.Statistics stats) public. Then, this is no longer an incompatible change. Note that removing public DFSClient(ClientProtocol namenode, ClientProtocol rpcNamenode, Configuration conf, FileSystem.Statistics stats) is fine since it is not yet released.
          Hide
          dhruba borthakur added a comment -

          I like this one.....better to expose fewer public APIs,

          Show
          dhruba borthakur added a comment - I like this one.....better to expose fewer public APIs,
          Hide
          Konstantin Boudnik added a comment -

          Would it be good also to use @link instead of

          +  /** Same as this(nameNodeAddr, conf, null); */
          
          Show
          Konstantin Boudnik added a comment - Would it be good also to use @link instead of + /** Same as this(nameNodeAddr, conf, null); */
          Hide
          Konstantin Boudnik added a comment -

          Would it be good also to use @link instead of
          noformat
          + /** Same as this(nameNodeAddr, conf, null); */
          noformat

          Show
          Konstantin Boudnik added a comment - Would it be good also to use @link instead of noformat + /** Same as this(nameNodeAddr, conf, null); */ noformat
          Hide
          Tsz Wo Nicholas Sze added a comment -

          h527_20090804c.patch: added a few @see tags in the DFSClient constructors (It is hard to specify the default values with @link tag.)

          Show
          Tsz Wo Nicholas Sze added a comment - h527_20090804c.patch: added a few @see tags in the DFSClient constructors (It is hard to specify the default values with @link tag.)
          Hide
          Konstantin Boudnik added a comment -

          +1 - patch looks good.

          Show
          Konstantin Boudnik added a comment - +1 - patch looks good.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Thanks, Cos for the review.

          Show
          Tsz Wo Nicholas Sze added a comment - Thanks, Cos for the review.
          Hide
          Tsz Wo Nicholas Sze added a comment -
               [exec] +1 overall.  
               [exec] 
               [exec]     +1 @author.  The patch does not contain any @author tags.
               [exec] 
               [exec]     +1 tests included.  The patch appears to include 12 new or modified tests.
               [exec] 
               [exec]     +1 javadoc.  The javadoc tool did not generate any warning messages.
               [exec] 
               [exec]     +1 javac.  The applied patch does not increase the total number of javac compiler warnings.
               [exec] 
               [exec]     +1 findbugs.  The patch does not introduce any new Findbugs warnings.
               [exec] 
               [exec]     +1 release audit.  The applied patch does not increase the total number of release audit warnings.
          

          Running unit tests.

          Show
          Tsz Wo Nicholas Sze added a comment - [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 12 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. Running unit tests.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Passed all unit tests locally.

          I have committed this.

          Show
          Tsz Wo Nicholas Sze added a comment - Passed all unit tests locally. I have committed this.
          Hide
          Hadoop QA added a comment -

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

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

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

          -1 patch. The patch command could not apply the patch.

          Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-vesta.apache.org/45/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/12415560/h527_20090804c.patch against trunk revision 801500. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-vesta.apache.org/45/console This message is automatically generated.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #41 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/41/)
          . Remove/deprecate unnecessary DFSClient constructors.

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #41 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/41/ ) . Remove/deprecate unnecessary DFSClient constructors.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Reopen for committing this to 0.20.

          Show
          Tsz Wo Nicholas Sze added a comment - Reopen for committing this to 0.20.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          h527_20090804c_0.20.patch: for 0.20

          Usually, we do not back-port code refactoring. However, this patch fixes some problems introduced by HDFS-167, which was back-ported to 0.20.

          Show
          Tsz Wo Nicholas Sze added a comment - h527_20090804c_0.20.patch: for 0.20 Usually, we do not back-port code refactoring. However, this patch fixes some problems introduced by HDFS-167 , which was back-ported to 0.20.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I have committed this to 0.20.

          Show
          Tsz Wo Nicholas Sze added a comment - I have committed this to 0.20.
          Hide
          Robert Chansler added a comment -

          Editorial pass over all release notes prior to publication of 0.21.

          Show
          Robert Chansler added a comment - Editorial pass over all release notes prior to publication of 0.21.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development