Hadoop Common
  1. Hadoop Common
  2. HADOOP-7316

Add public javadocs to FSDataInputStream and FSDataOutputStream

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.23.0
    • Component/s: documentation
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      This is a method made public for testing. In comments in HADOOP-7301 after commit, adding javadoc comments was requested. This is a follow up jira to address it.

      1. hadoop-7316-1.patch
        6 kB
        Eli Collins
      2. hadoop-7316-2.patch
        6 kB
        Eli Collins
      3. hadoop-7316-3.patch
        6 kB
        Eli Collins

        Issue Links

          Activity

          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Patch Available Patch Available Open Open
          6h 51m 1 Owen O'Malley 22/May/11 16:27
          Open Open Patch Available Patch Available
          13d 15h 41m 2 Eli Collins 03/Jun/11 22:02
          Patch Available Patch Available Resolved Resolved
          22m 49s 1 Eli Collins 03/Jun/11 22:25
          Resolved Resolved Closed Closed
          164d 2h 24m 1 Arun C Murthy 15/Nov/11 00:50
          Gavin made changes -
          Link This issue depends upon HADOOP-7301 [ HADOOP-7301 ]
          Gavin made changes -
          Link This issue depends on HADOOP-7301 [ HADOOP-7301 ]
          Arun C Murthy made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #709 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk/709/)
          HADOOP-7316. Add public javadocs to FSDataInputStream and FSDataOutputStream. Contributed by Eli Collins

          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1131268
          Files :

          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/FsCommand.java
          • /hadoop/common/trunk/CHANGES.txt
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Command.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/PositionedReadable.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/FSDataOutputStream.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/FSDataInputStream.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #709 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk/709/ ) HADOOP-7316 . Add public javadocs to FSDataInputStream and FSDataOutputStream. Contributed by Eli Collins eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1131268 Files : /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/FsCommand.java /hadoop/common/trunk/CHANGES.txt /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Command.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/PositionedReadable.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/FSDataOutputStream.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/FSDataInputStream.java
          Hide
          John George added a comment -

          >> ... Because Daryn and I were both confused about the behavior ...

          > Hi John, I think the current behavior is not well designed. If it is the case, you may also fix it.

          Thanks Nicholas. I created HDFS-2033 to track read path design.

          Show
          John George added a comment - >> ... Because Daryn and I were both confused about the behavior ... > Hi John, I think the current behavior is not well designed. If it is the case, you may also fix it. Thanks Nicholas. I created HDFS-2033 to track read path design.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > ... Because Daryn and I were both confused about the behavior ...

          Hi John, I think the current behavior is not well designed. If it is the case, you may also fix it.

          Show
          Tsz Wo Nicholas Sze added a comment - > ... Because Daryn and I were both confused about the behavior ... Hi John, I think the current behavior is not well designed. If it is the case, you may also fix it.
          Hide
          Eli Collins added a comment -

          Go for it!

          Show
          Eli Collins added a comment - Go for it!
          Hide
          John George added a comment -

          Thanks a lot Todd and Eli.
          Is it okay if I add this as a comment to DFSInputStream at some point? Because Daryn and I were both confused about the behavior and the above information really helps.

          Show
          John George added a comment - Thanks a lot Todd and Eli. Is it okay if I add this as a comment to DFSInputStream at some point? Because Daryn and I were both confused about the behavior and the above information really helps.
          Hide
          Todd Lipcon added a comment -

          DFSInputStream#seek should really throw an EOFException if seeking beyond the end of stream, but changing it to do that now would break compatibility =(

          Actually, Java's RandomAccessFile API lets you seek way past the end of a file with no exception. The next read() call will simply return -1. But asking for "position()" on the file's channel continues to return the past-eof offset.

          Show
          Todd Lipcon added a comment - DFSInputStream#seek should really throw an EOFException if seeking beyond the end of stream, but changing it to do that now would break compatibility =( Actually, Java's RandomAccessFile API lets you seek way past the end of a file with no exception. The next read() call will simply return -1. But asking for "position()" on the file's channel continues to return the past-eof offset.
          Hide
          Eli Collins added a comment -

          Depends on the underlying InputStream. read (and readFully) result in a call to seek, which in the case of DFSInputStream throws an IOException if you try to seek past the length of the files. FSInputStream#readFully however throws an EOFException if the underlying call to read failed while FSInputStream#read will return whatever value the underlying call to read returned (will not throw an EOFException). So, per the javadoc, you only get the EOFException with readFully if you hit the EOF while reading.

          DFSInputStream#seek should really throw an EOFException if seeking beyond the end of stream, but changing it to do that now would break compatibility =(

          Show
          Eli Collins added a comment - Depends on the underlying InputStream. read (and readFully) result in a call to seek, which in the case of DFSInputStream throws an IOException if you try to seek past the length of the files. FSInputStream#readFully however throws an EOFException if the underlying call to read failed while FSInputStream#read will return whatever value the underlying call to read returned (will not throw an EOFException). So, per the javadoc, you only get the EOFException with readFully if you hit the EOF while reading. DFSInputStream#seek should really throw an EOFException if seeking beyond the end of stream, but changing it to do that now would break compatibility =(
          Hide
          John George added a comment -

          Just to understand this a little better:
          Is the read() function also not expected to throw an EOF Exception if the user tried to read beyond EOF?

          Show
          John George added a comment - Just to understand this a little better: Is the read() function also not expected to throw an EOF Exception if the user tried to read beyond EOF?
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #638 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk-Commit/638/)
          HADOOP-7316. Add public javadocs to FSDataInputStream and FSDataOutputStream. Contributed by Eli Collins

          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1131268
          Files :

          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/FsCommand.java
          • /hadoop/common/trunk/CHANGES.txt
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Command.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/PositionedReadable.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/FSDataOutputStream.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/FSDataInputStream.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #638 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk-Commit/638/ ) HADOOP-7316 . Add public javadocs to FSDataInputStream and FSDataOutputStream. Contributed by Eli Collins eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1131268 Files : /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/FsCommand.java /hadoop/common/trunk/CHANGES.txt /hadoop/common/trunk/src/java/org/apache/hadoop/fs/shell/Command.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/PositionedReadable.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/FSDataOutputStream.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/FSDataInputStream.java
          Eli Collins made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags [Reviewed]
          Resolution Fixed [ 1 ]
          Hide
          Eli Collins added a comment -

          Thanks Todd! I've committed this.

          Show
          Eli Collins added a comment - Thanks Todd! I've 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/12481412/hadoop-7316-3.patch
          against trunk revision 1131254.

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

          +0 tests included. The patch appears to be a documentation patch that doesn't require tests.

          +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 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 passed core unit tests.

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/575//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/575//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/575//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/12481412/hadoop-7316-3.patch against trunk revision 1131254. +1 @author. The patch does not contain any @author tags. +0 tests included. The patch appears to be a documentation patch that doesn't require tests. +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 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 passed core unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/575//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/575//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/575//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          +1 pending hudson

          Show
          Todd Lipcon added a comment - +1 pending hudson
          Eli Collins made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Eli Collins made changes -
          Attachment hadoop-7316-3.patch [ 12481412 ]
          Hide
          Eli Collins added a comment -

          Thanks Todd. Updated patch attached.

          Show
          Eli Collins added a comment - Thanks Todd. Updated patch attached.
          Hide
          Todd Lipcon added a comment -

          Super nitty review on the javadoc:

          • Javadoc style guide at http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html says the following about punctuation and capitalization for @param and @return:

            The description begins with a lowercase letter if it is a phrase (contains no verb), or an uppercase letter if it is a sentence. End the phrase with a period only if another phrase or sentence follows it.

            (ie the @param and @returns should start with lower case letters)

          • following should read "the buffer into which data is read" in two places:
            + * @param buffer The buffer in which data is read.
          • for the readFully javadoc, @param length should say "the exact number of bytes to read" or "the number of bytes" rather than "the maximum" given the contract of readFully.
          • for readFully, we should also specify that, even if it throws an exception, it may modify an undetermined number of bytes in the target buffer.
          • typo: "altenate" should be "alternate"
          Show
          Todd Lipcon added a comment - Super nitty review on the javadoc: Javadoc style guide at http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html says the following about punctuation and capitalization for @param and @return: The description begins with a lowercase letter if it is a phrase (contains no verb), or an uppercase letter if it is a sentence. End the phrase with a period only if another phrase or sentence follows it. (ie the @param and @returns should start with lower case letters) following should read "the buffer into which data is read" in two places: + * @param buffer The buffer in which data is read. for the readFully javadoc, @param length should say "the exact number of bytes to read" or "the number of bytes" rather than "the maximum" given the contract of readFully. for readFully, we should also specify that, even if it throws an exception, it may modify an undetermined number of bytes in the target buffer. typo: "altenate" should be "alternate"
          Hide
          Eli Collins added a comment -

          Hey Owen - is the latest patch acceptable to you?

          Show
          Eli Collins added a comment - Hey Owen - is the latest patch acceptable to you?
          Eli Collins made changes -
          Attachment hadoop-7316-2.patch [ 12480132 ]
          Hide
          Eli Collins added a comment -

          Thanks Owen. Updated patch attached.

          I marked both FSDataInputStream and FSDataOutputStream#getWrappedStream LimitedPrivate to HDFS.

          The addition of FSDataInputStream#getWrappedStream mirrors the same method in FSDataOutputStream. Both live in common but are just used for HDFS tests, which is why they are not package visible. Could also expose these via a test adapter class but I think the annotation is sufficient.

          Show
          Eli Collins added a comment - Thanks Owen. Updated patch attached. I marked both FSDataInputStream and FSDataOutputStream#getWrappedStream LimitedPrivate to HDFS. The addition of FSDataInputStream#getWrappedStream mirrors the same method in FSDataOutputStream. Both live in common but are just used for HDFS tests, which is why they are not package visible. Could also expose these via a test adapter class but I think the annotation is sufficient.
          Tsz Wo Nicholas Sze made changes -
          Issue Type New Feature [ 2 ] Improvement [ 4 ]
          Component/s documentation [ 12311160 ]
          Owen O'Malley made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Owen O'Malley added a comment -

          The newly visible method should be annotated with interface audience private.

          Why was it necessary to make this method public instead of package visible? All testing should be able to use that.

          Show
          Owen O'Malley added a comment - The newly visible method should be annotated with interface audience private. Why was it necessary to make this method public instead of package visible? All testing should be able to use that.
          Eli Collins made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Eli Collins made changes -
          Attachment hadoop-7316-1.patch [ 12480030 ]
          Hide
          Eli Collins added a comment -

          Patch attached. Also fixes a couple minor javadoc errors in FsCommand and Command.

          Show
          Eli Collins added a comment - Patch attached. Also fixes a couple minor javadoc errors in FsCommand and Command.
          Eli Collins made changes -
          Summary Add javadoc comments to FSDataInputStream.getWrappedStream public method. Add public javadocs to FSDataInputStream and FSDataOutputStream
          Assignee Eli Collins [ eli ]
          Hide
          Eli Collins added a comment -

          Might as well do this for all public methods in both FSDataInputStream and FSDataOutputStream.

          Show
          Eli Collins added a comment - Might as well do this for all public methods in both FSDataInputStream and FSDataOutputStream.
          Hide
          Jonathan Hsieh added a comment -

          As requested by Nicholas

          Show
          Jonathan Hsieh added a comment - As requested by Nicholas
          Jonathan Hsieh made changes -
          Field Original Value New Value
          Link This issue depends on HADOOP-7301 [ HADOOP-7301 ]
          Jonathan Hsieh created issue -

            People

            • Assignee:
              Eli Collins
              Reporter:
              Jonathan Hsieh
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development