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-3.patch
        6 kB
        Eli Collins
      2. hadoop-7316-2.patch
        6 kB
        Eli Collins
      3. hadoop-7316-1.patch
        6 kB
        Eli Collins

        Issue Links

          Activity

          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
          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
          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?
          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.
          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.
          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.
          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

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development