Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-6369

Document that BlockReader#available() can return more bytes than are remaining in the block

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.5.0
    • Component/s: None
    • Labels:
      None
    • Target Version/s:

      Description

      Let's document that BlockReader#available() can return more bytes than are remaining in the block.

      1. HDFS-6369.patch
        0.7 kB
        Chen He
      2. hdfs-6369-v2.txt
        0.7 kB
        Ted Yu
      3. hdfs-6369-v3.txt
        0.6 kB
        Ted Yu

        Activity

        Hide
        Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk #1793 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1793/)
        HDFS-6369. Document that BlockReader#available() can return more bytes than are remaining in the block (Ted Yu via Colin Patrick McCabe) (cmccabe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1600801)

        • /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/BlockReader.java
        Show
        Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #1793 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1793/ ) HDFS-6369 . Document that BlockReader#available() can return more bytes than are remaining in the block (Ted Yu via Colin Patrick McCabe) (cmccabe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1600801 ) /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/BlockReader.java
        Hide
        Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk #1766 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1766/)
        HDFS-6369. Document that BlockReader#available() can return more bytes than are remaining in the block (Ted Yu via Colin Patrick McCabe) (cmccabe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1600801)

        • /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/BlockReader.java
        Show
        Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #1766 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1766/ ) HDFS-6369 . Document that BlockReader#available() can return more bytes than are remaining in the block (Ted Yu via Colin Patrick McCabe) (cmccabe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1600801 ) /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/BlockReader.java
        Hide
        Hudson added a comment -

        FAILURE: Integrated in Hadoop-Yarn-trunk #575 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/575/)
        HDFS-6369. Document that BlockReader#available() can return more bytes than are remaining in the block (Ted Yu via Colin Patrick McCabe) (cmccabe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1600801)

        • /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/BlockReader.java
        Show
        Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #575 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/575/ ) HDFS-6369 . Document that BlockReader#available() can return more bytes than are remaining in the block (Ted Yu via Colin Patrick McCabe) (cmccabe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1600801 ) /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/BlockReader.java
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in Hadoop-trunk-Commit #5652 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5652/)
        HDFS-6369. Document that BlockReader#available() can return more bytes than are remaining in the block (Ted Yu via Colin Patrick McCabe) (cmccabe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1600801)

        • /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/BlockReader.java
        Show
        Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #5652 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5652/ ) HDFS-6369 . Document that BlockReader#available() can return more bytes than are remaining in the block (Ted Yu via Colin Patrick McCabe) (cmccabe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1600801 ) /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/BlockReader.java
        Hide
        Colin Patrick McCabe added a comment -

        committed, thanks.

        Show
        Colin Patrick McCabe added a comment - committed, thanks.
        Hide
        Colin Patrick McCabe added a comment -

        +1. No tests needed, since this is just a docs change.

        Show
        Colin Patrick McCabe added a comment - +1. No tests needed, since this is just a docs change.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12648536/hdfs-6369-v3.txt
        against trunk revision .

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

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

        +1 javadoc. There were no new javadoc warning messages.

        +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 passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

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

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/7039//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/7039//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/12648536/hdfs-6369-v3.txt against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +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 passed unit tests in hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/7039//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/7039//console This message is automatically generated.
        Hide
        Colin Patrick McCabe added a comment -

        Thanks for this, Ted. Only one comment: I think it would make more sense to add this for the JavaDoc for the base class (BlockReader).

        Show
        Colin Patrick McCabe added a comment - Thanks for this, Ted. Only one comment: I think it would make more sense to add this for the JavaDoc for the base class ( BlockReader ).
        Hide
        Colin Patrick McCabe added a comment -

        As I said earlier, the calling code checks how many bytes are left in the block. So unless I'm missing something, doing this computation just creates extra work, but has no benefit. Another concern is that you only changed the deprecated RemoteBlockReader class, and not the RemoteBlockReader2 class which almost everyone has transitioned to.

        How about we should just add a comment on available that it may return more than is actually present in the block? That will make it easier to read the code.

        Show
        Colin Patrick McCabe added a comment - As I said earlier, the calling code checks how many bytes are left in the block. So unless I'm missing something, doing this computation just creates extra work, but has no benefit. Another concern is that you only changed the deprecated RemoteBlockReader class, and not the RemoteBlockReader2 class which almost everyone has transitioned to. How about we should just add a comment on available that it may return more than is actually present in the block? That will make it easier to read the code.
        Hide
        Chen He added a comment -

        Hi Colin Patrick McCabe, thank you for the comments, I will update the patch.

        Show
        Chen He added a comment - Hi Colin Patrick McCabe , thank you for the comments, I will update the patch.
        Hide
        Hadoop QA added a comment -

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

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

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

        +1 javadoc. There were no new javadoc warning messages.

        +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 passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

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

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6878//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6878//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/12644364/HDFS-6369.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +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 passed unit tests in hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6878//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6878//console This message is automatically generated.
        Hide
        Colin Patrick McCabe added a comment -

        The calling code in DFSInputStream already takes into account where the end of the block is. See this code in DFSInputStream:

            if (pos <= targetPos && targetPos <= blockEnd) {
              //
              // If this seek is to a positive position in the current
              // block, and this piece of data might already be lying in
              // the TCP buffer, then just eat up the intervening data.
              //
              int diff = (int)(targetPos - pos);
              if (diff <= blockReader.available()) {
              ...
        

        Notice that we don't call available if targetPos is beyond blockEnd

        Also, RemoteBlockReader is deprecated. See the comment at the top:

        /**
         * \@deprecated this is an old implementation that is being left around
         * in case any issues spring up with the new {\@link RemoteBlockReader2} implementation.
         * It will be removed in the next release.
         */
        

        We should really fix RemoteBlockReader2 to support SOCKS sockets (those are the only ones without channels, as I recall) and remove RemoteBlockReader as the comment promised we'd do.

        Show
        Colin Patrick McCabe added a comment - The calling code in DFSInputStream already takes into account where the end of the block is. See this code in DFSInputStream : if (pos <= targetPos && targetPos <= blockEnd) { // // If this seek is to a positive position in the current // block, and this piece of data might already be lying in // the TCP buffer, then just eat up the intervening data. // int diff = ( int )(targetPos - pos); if (diff <= blockReader.available()) { ... Notice that we don't call available if targetPos is beyond blockEnd Also, RemoteBlockReader is deprecated. See the comment at the top: /** * \@deprecated this is an old implementation that is being left around * in case any issues spring up with the new {\@link RemoteBlockReader2} implementation. * It will be removed in the next release. */ We should really fix RemoteBlockReader2 to support SOCKS sockets (those are the only ones without channels, as I recall) and remove RemoteBlockReader as the comment promised we'd do.
        Hide
        Chen He added a comment -

        This is interesting, I will take it.

        Show
        Chen He added a comment - This is interesting, I will take it.

          People

          • Assignee:
            Ted Yu
            Reporter:
            Ted Yu
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development