Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.0
    • Fix Version/s: 2.3.0
    • Component/s: hdfs-client
    • Labels:
      None
    • Target Version/s:

      Description

      BlockReaderLocal should be able to switch between checksumming and non-checksumming, so that when we get notifications that something is mlocked (see HDFS-5182), we can avoid checksumming when reading from that block.

      1. HDFS-5634.001.patch
        63 kB
        Colin Patrick McCabe
      2. HDFS-5634.002.patch
        64 kB
        Colin Patrick McCabe
      3. HDFS-5634.003.patch
        66 kB
        Colin Patrick McCabe
      4. HDFS-5634.004.patch
        66 kB
        Colin Patrick McCabe
      5. HDFS-5634.005.patch
        79 kB
        Colin Patrick McCabe
      6. HDFS-5634.006.patch
        80 kB
        Colin Patrick McCabe
      7. HDFS-5634.007.patch
        72 kB
        Colin Patrick McCabe
      8. HDFS-5634.008.patch
        72 kB
        Colin Patrick McCabe
      9. HDFS-5634.009.branch-2.patch
        71 kB
        Colin Patrick McCabe

        Issue Links

          Activity

          Hide
          Colin Patrick McCabe added a comment -

          backported to 2.4

          Show
          Colin Patrick McCabe added a comment - backported to 2.4
          Hide
          Colin Patrick McCabe added a comment -

          posted patch for branch-2

          Show
          Colin Patrick McCabe added a comment - posted patch for branch-2
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Mapreduce-trunk #1642 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1642/)
          HDFS-5634. Allow BlockReaderLocal to switch between checksumming and not (cmccabe) (cmccabe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1551701)

          • /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
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderFactory.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocalLegacy.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockMetadataHeader.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestShortCircuitLocalRead.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #1642 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1642/ ) HDFS-5634 . Allow BlockReaderLocal to switch between checksumming and not (cmccabe) (cmccabe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1551701 ) /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 /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderFactory.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocalLegacy.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockMetadataHeader.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestShortCircuitLocalRead.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Hdfs-trunk #1616 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1616/)
          HDFS-5634. Allow BlockReaderLocal to switch between checksumming and not (cmccabe) (cmccabe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1551701)

          • /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
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderFactory.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocalLegacy.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockMetadataHeader.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestShortCircuitLocalRead.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #1616 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1616/ ) HDFS-5634 . Allow BlockReaderLocal to switch between checksumming and not (cmccabe) (cmccabe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1551701 ) /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 /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderFactory.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocalLegacy.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockMetadataHeader.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestShortCircuitLocalRead.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Yarn-trunk #425 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/425/)
          HDFS-5634. Allow BlockReaderLocal to switch between checksumming and not (cmccabe) (cmccabe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1551701)

          • /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
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderFactory.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocalLegacy.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockMetadataHeader.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestShortCircuitLocalRead.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #425 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/425/ ) HDFS-5634 . Allow BlockReaderLocal to switch between checksumming and not (cmccabe) (cmccabe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1551701 ) /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 /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderFactory.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocalLegacy.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockMetadataHeader.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestShortCircuitLocalRead.java
          Hide
          Hadoop QA added a comment -

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

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

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

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

          -1 javadoc. The javadoc tool appears to have generated -14 warning messages.

          -1 eclipse:eclipse. The patch failed to build 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 in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.web.TestWebHDFS

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5743//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5743//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/12619139/HDFS-5634.008.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 javadoc . The javadoc tool appears to have generated -14 warning messages. -1 eclipse:eclipse . The patch failed to build 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 in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.web.TestWebHDFS +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5743//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5743//console This message is automatically generated.
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-trunk-Commit #4902 (See https://builds.apache.org/job/Hadoop-trunk-Commit/4902/)
          HDFS-5634. Allow BlockReaderLocal to switch between checksumming and not (cmccabe) (cmccabe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1551701)

          • /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
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderFactory.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocalLegacy.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockMetadataHeader.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestShortCircuitLocalRead.java
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #4902 (See https://builds.apache.org/job/Hadoop-trunk-Commit/4902/ ) HDFS-5634 . Allow BlockReaderLocal to switch between checksumming and not (cmccabe) (cmccabe: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1551701 ) /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 /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderFactory.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocalLegacy.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockMetadataHeader.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestShortCircuitLocalRead.java
          Hide
          Hadoop QA added a comment -

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

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

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

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

          +1 javadoc. The javadoc tool did not generate any 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/5742//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5742//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/12619139/HDFS-5634.008.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any 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/5742//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5742//console This message is automatically generated.
          Hide
          Colin Patrick McCabe added a comment -

          yeah, I suppose it should be needed >= maxReadaheadLength. Having being able to read exactly the readahead length through the fast path should be allowed.

          Show
          Colin Patrick McCabe added a comment - yeah, I suppose it should be needed >= maxReadaheadLength. Having being able to read exactly the readahead length through the fast path should be allowed.
          Hide
          Andrew Wang added a comment -

          +1, thanks for all the revs Colin. If we find any more issues we can fix them in post.

          One last question, you can address it here or in the pending follow on:

                } else if (buf.isDirect() && (needed > maxReadaheadLength)
          

          Should this be needed >= maxReadaheadLength ?

          Show
          Andrew Wang added a comment - +1, thanks for all the revs Colin. If we find any more issues we can fix them in post. One last question, you can address it here or in the pending follow on: } else if (buf.isDirect() && (needed > maxReadaheadLength) Should this be needed >= maxReadaheadLength ?
          Hide
          Hadoop QA added a comment -

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

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

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

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

          +1 javadoc. The javadoc tool did not generate any 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/5736//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5736//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/12618992/HDFS-5634.007.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any 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/5736//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5736//console This message is automatically generated.
          Hide
          Colin Patrick McCabe added a comment -

          Another problem related to changing verifyChecksum to skipChecksum. I think, given the annoyance of doing that, I'm just going to leave everything as verifyChecksum. This will also make merging to other branches easier. Will post patch in a second, once I re-run tests.

          Show
          Colin Patrick McCabe added a comment - Another problem related to changing verifyChecksum to skipChecksum. I think, given the annoyance of doing that, I'm just going to leave everything as verifyChecksum. This will also make merging to other branches easier. Will post patch in a second, once I re-run tests.
          Hide
          Hadoop QA added a comment -

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

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

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

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

          +1 javadoc. The javadoc tool did not generate any 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 failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.TestClientBlockVerification

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5733//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5733//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/12618950/HDFS-5634.006.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any 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 failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestClientBlockVerification +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5733//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5733//console This message is automatically generated.
          Hide
          Colin Patrick McCabe added a comment -
          • Fixed a bug introduced by converting to using skipChecksums in some places.
          • Ensure that the "fast path" starts reading on a checksum boundary.
          Show
          Colin Patrick McCabe added a comment - Fixed a bug introduced by converting to using skipChecksums in some places. Ensure that the "fast path" starts reading on a checksum boundary.
          Hide
          Colin Patrick McCabe added a comment -

          found a bug in the fastpath handling. will post an update soon

          Show
          Colin Patrick McCabe added a comment - found a bug in the fastpath handling. will post an update soon
          Hide
          Hadoop QA added a comment -

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

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

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

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

          +1 javadoc. The javadoc tool did not generate any 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 failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.TestCrcCorruption
          org.apache.hadoop.hdfs.TestClientReportBadBlock
          org.apache.hadoop.hdfs.TestDFSClientRetries
          org.apache.hadoop.hdfs.server.namenode.TestNameNodeHttpServer
          org.apache.hadoop.hdfs.server.namenode.TestCorruptFilesJsp
          org.apache.hadoop.hdfs.server.blockmanagement.TestBlocksWithNotEnoughRacks
          org.apache.hadoop.hdfs.server.namenode.TestFsck
          org.apache.hadoop.hdfs.server.namenode.TestListCorruptFileBlocks
          org.apache.hadoop.hdfs.TestDFSShell

          The following test timeouts occurred in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.TestDatanodeBlockScanner

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5714//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5714//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/12618694/HDFS-5634.005.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any 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 failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestCrcCorruption org.apache.hadoop.hdfs.TestClientReportBadBlock org.apache.hadoop.hdfs.TestDFSClientRetries org.apache.hadoop.hdfs.server.namenode.TestNameNodeHttpServer org.apache.hadoop.hdfs.server.namenode.TestCorruptFilesJsp org.apache.hadoop.hdfs.server.blockmanagement.TestBlocksWithNotEnoughRacks org.apache.hadoop.hdfs.server.namenode.TestFsck org.apache.hadoop.hdfs.server.namenode.TestListCorruptFileBlocks org.apache.hadoop.hdfs.TestDFSShell The following test timeouts occurred in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestDatanodeBlockScanner +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5714//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5714//console This message is automatically generated.
          Hide
          Colin Patrick McCabe added a comment -

          I addressed the stuff discussed above. I also addressed the case where there are no checksums for the block. This can lead to some weird conditions like bytesPerChecksum being 0, so we want to avoid dividing by this number before we check it. This patch also sets the skipChecksum boolean when it detects one of these blocks, which should speed things up. This is an optimization that the old code didn't do.

          BlockReaderLocalTest is now an abstract class rather than an interface, so that we can have defaults. I added a bunch of new tests, including one where we set readahead just less than the block size.

          Show
          Colin Patrick McCabe added a comment - I addressed the stuff discussed above. I also addressed the case where there are no checksums for the block. This can lead to some weird conditions like bytesPerChecksum being 0, so we want to avoid dividing by this number before we check it. This patch also sets the skipChecksum boolean when it detects one of these blocks, which should speed things up. This is an optimization that the old code didn't do. BlockReaderLocalTest is now an abstract class rather than an interface, so that we can have defaults. I added a bunch of new tests, including one where we set readahead just less than the block size.
          Hide
          Colin Patrick McCabe added a comment -

          Alright. It seems odd for datanodeID and block to ever be null since they're used to key the FISCache, but okay. Still, we're currently not setting the caching strategy in DFSInputStream#getBlockReader when pulling something out of the FISCache, should we?

          Definitely. Fixed.

          In fillDataBuf(boolean), it looks like we try to fill maxReadaheadLength modulo the slop. If this is zero, what happens? I think we need to take the checksum chunk size into account here somewhere.

          This is a bug. Basically it's a corner case where we need to fudge the user's no-readahead request into a short-readahead request when doing a checksummed read. Will fix...

          It'd also be good to have test coverage for all these different read paths and parameter combinations. It looks like TestParallel* already cover the ByteBuffer and array variants of read with and without checksums, but we also need to permute on different readahead sizes (e.g. 0, chunk-1, chunk+1). Unfortunately even though the array and BB versions are almost the same, they don't share any code, so we should probably still permute on that axis.

          I think the main thing is to have coverage for the no-readahead + checksum case, which we don't really now. Having a test that did chunk-1 readahead, to verify that the round-up functionality worked, would also be a good idea.

          With the new getter, we could use that instead of passing around canSkipChecksum everywhere, but I'm unsure if mlock toggling has implications here. Your call.

          Accessing the atomic boolean does have a performance overhead that I wanted to avoid. Also, as you said, it might change, which could create problems.

          I was thinking user documentation in hdfs-default.xml for "dfs.client.read.shortcircuit.buffer.size" and "dfs.datanode.readahead.bytes" but I see you filed a follow-on for that. It also feels inconsistent that the datanode defaults to 4MB readahead while the client looks like it defaults to 0, but maybe there's some reasoning there.

          Actually, the client defaults to "null" readahead which usually means the datanode gets to decide. I guess it's a bit of a confusing construct, but here it is:

              Long readahead = (conf.get(DFS_CLIENT_CACHE_READAHEAD) == null) ?
                  null : conf.getLong(DFS_CLIENT_CACHE_READAHEAD, 0);
          

          In BlockReaderLocal I am defaulting to dfs.datanode.readahead.bytes when the client's value is null. If we wanted to be totally consistent with how remote reads work, we'd have to get the readahead default from the DataNode as part of the RPC. That seems a little bit like overkill, though.

          There's still BlockReaderFactory#newBlockReader using verifyChecksum rather than skipChecksum. Do you want to change this one too? There are only 6 usages from what I see.

          OK.

          Show
          Colin Patrick McCabe added a comment - Alright. It seems odd for datanodeID and block to ever be null since they're used to key the FISCache, but okay. Still, we're currently not setting the caching strategy in DFSInputStream#getBlockReader when pulling something out of the FISCache, should we? Definitely. Fixed. In fillDataBuf(boolean), it looks like we try to fill maxReadaheadLength modulo the slop. If this is zero, what happens? I think we need to take the checksum chunk size into account here somewhere. This is a bug. Basically it's a corner case where we need to fudge the user's no-readahead request into a short-readahead request when doing a checksummed read. Will fix... It'd also be good to have test coverage for all these different read paths and parameter combinations. It looks like TestParallel* already cover the ByteBuffer and array variants of read with and without checksums, but we also need to permute on different readahead sizes (e.g. 0, chunk-1, chunk+1). Unfortunately even though the array and BB versions are almost the same, they don't share any code, so we should probably still permute on that axis. I think the main thing is to have coverage for the no-readahead + checksum case, which we don't really now. Having a test that did chunk-1 readahead, to verify that the round-up functionality worked, would also be a good idea. With the new getter, we could use that instead of passing around canSkipChecksum everywhere, but I'm unsure if mlock toggling has implications here. Your call. Accessing the atomic boolean does have a performance overhead that I wanted to avoid. Also, as you said, it might change, which could create problems. I was thinking user documentation in hdfs-default.xml for "dfs.client.read.shortcircuit.buffer.size" and "dfs.datanode.readahead.bytes" but I see you filed a follow-on for that. It also feels inconsistent that the datanode defaults to 4MB readahead while the client looks like it defaults to 0, but maybe there's some reasoning there. Actually, the client defaults to "null" readahead which usually means the datanode gets to decide. I guess it's a bit of a confusing construct, but here it is: Long readahead = (conf.get(DFS_CLIENT_CACHE_READAHEAD) == null ) ? null : conf.getLong(DFS_CLIENT_CACHE_READAHEAD, 0); In BlockReaderLocal I am defaulting to dfs.datanode.readahead.bytes when the client's value is null. If we wanted to be totally consistent with how remote reads work, we'd have to get the readahead default from the DataNode as part of the RPC. That seems a little bit like overkill, though. There's still BlockReaderFactory#newBlockReader using verifyChecksum rather than skipChecksum. Do you want to change this one too? There are only 6 usages from what I see. OK.
          Hide
          Andrew Wang added a comment -

          Thanks for being patient with me, I think I finally grok what's going on.

          <BRL builder>

          Alright. It seems odd for datanodeID and block to ever be null since they're used to key the FISCache, but okay. Still, we're currently not setting the caching strategy in DFSInputStream#getBlockReader when pulling something out of the FISCache, should we?

          We always buffer at least a single chunk, even if readahead is turned off. The mechanics of checksumming require this.

          In fillDataBuf(boolean), it looks like we try to fill maxReadaheadLength modulo the slop. If this is zero, what happens? I think we need to take the checksum chunk size into account here somewhere.

          It'd also be good to have test coverage for all these different read paths and parameter combinations. It looks like TestParallel* already cover the ByteBuffer and array variants of read with and without checksums, but we also need to permute on different readahead sizes (e.g. 0, chunk-1, chunk+1). Unfortunately even though the array and BB versions are almost the same, they don't share any code, so we should probably still permute on that axis.

          With the new getter, we could use that instead of passing around canSkipChecksum everywhere, but I'm unsure if mlock toggling has implications here. Your call.

          There's still BlockReaderFactory#newBlockReader using verifyChecksum rather than skipChecksum. Do you want to change this one too? There are only 6 usages from what I see.

          <deque>

          Yea, let's just skip this idea for now. We really should use a profiler for guidance before trying optimizations anyway. That'd be interesting to do later.

          <readahead and buffer size linkage>

          I was thinking user documentation in hdfs-default.xml for "dfs.client.read.shortcircuit.buffer.size" and "dfs.datanode.readahead.bytes" but I see you filed a follow-on for that. It also feels inconsistent that the datanode defaults to 4MB readahead while the client looks like it defaults to 0, but maybe there's some reasoning there.

          Show
          Andrew Wang added a comment - Thanks for being patient with me, I think I finally grok what's going on. <BRL builder> Alright. It seems odd for datanodeID and block to ever be null since they're used to key the FISCache, but okay. Still, we're currently not setting the caching strategy in DFSInputStream#getBlockReader when pulling something out of the FISCache, should we? We always buffer at least a single chunk, even if readahead is turned off. The mechanics of checksumming require this. In fillDataBuf(boolean) , it looks like we try to fill maxReadaheadLength modulo the slop. If this is zero, what happens? I think we need to take the checksum chunk size into account here somewhere. It'd also be good to have test coverage for all these different read paths and parameter combinations. It looks like TestParallel* already cover the ByteBuffer and array variants of read with and without checksums, but we also need to permute on different readahead sizes (e.g. 0, chunk-1, chunk+1). Unfortunately even though the array and BB versions are almost the same, they don't share any code, so we should probably still permute on that axis. With the new getter, we could use that instead of passing around canSkipChecksum everywhere, but I'm unsure if mlock toggling has implications here. Your call. There's still BlockReaderFactory#newBlockReader using verifyChecksum rather than skipChecksum. Do you want to change this one too? There are only 6 usages from what I see. <deque> Yea, let's just skip this idea for now. We really should use a profiler for guidance before trying optimizations anyway. That'd be interesting to do later. <readahead and buffer size linkage> I was thinking user documentation in hdfs-default.xml for "dfs.client.read.shortcircuit.buffer.size" and "dfs.datanode.readahead.bytes" but I see you filed a follow-on for that. It also feels inconsistent that the datanode defaults to 4MB readahead while the client looks like it defaults to 0, but maybe there's some reasoning there.
          Hide
          Colin Patrick McCabe added a comment -

          I examined this code more carefully, and I found that it was actually using LIFO at the moment.

          Err, sorry, I mis-spoke. It is FIFO. Unfortunately, ConcurrentLinkedDeque is not in JDK6 (although it is in JDK7), so it will be hard to test with LIFO here.

          Show
          Colin Patrick McCabe added a comment - I examined this code more carefully, and I found that it was actually using LIFO at the moment. Err, sorry, I mis-spoke. It is FIFO. Unfortunately, ConcurrentLinkedDeque is not in JDK6 (although it is in JDK7), so it will be hard to test with LIFO here.
          Hide
          Hadoop QA added a comment -

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

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

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

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

          +1 javadoc. The javadoc tool did not generate any 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 failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.server.datanode.TestBPOfferService

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5698//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5698//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/12618311/HDFS-5634.004.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any 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 failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.server.datanode.TestBPOfferService +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5698//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5698//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

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

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

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

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

          +1 javadoc. The javadoc tool did not generate any 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/5696//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5696//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/12618297/HDFS-5634.003.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any 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/5696//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5696//console This message is automatically generated.
          Hide
          Colin Patrick McCabe added a comment -

          I optimized the CPU consumption a bit by caching the checksum size and bytes-per in final ints, and avoiding the need to re-do some multiplications a few times on every read. "perf stat" now gives me 305,384,306,460 cycles for TestParallelShortCircuitRead, as opposed to 321,040,227,686 cycles before.

          Show
          Colin Patrick McCabe added a comment - I optimized the CPU consumption a bit by caching the checksum size and bytes-per in final ints, and avoiding the need to re-do some multiplications a few times on every read. "perf stat" now gives me 305,384,306,460 cycles for TestParallelShortCircuitRead, as opposed to 321,040,227,686 cycles before.
          Hide
          Colin Patrick McCabe added a comment -

          DirectBufferPool, would it be better to use a Deque's stack operations rather than a Queue? Might give better cache locality to do LIFO rather than FIFO.

          I examined this code more carefully, and I found that it was actually using LIFO at the moment. The reason is because it uses ConcurrentLinkedQueue#add to add the elements, which add them to the end. It then uses ConcurrentLinkedQueue#poll to get the elements, which gets them from the beginning.

          Show
          Colin Patrick McCabe added a comment - DirectBufferPool, would it be better to use a Deque's stack operations rather than a Queue? Might give better cache locality to do LIFO rather than FIFO. I examined this code more carefully, and I found that it was actually using LIFO at the moment. The reason is because it uses ConcurrentLinkedQueue#add to add the elements, which add them to the end. It then uses ConcurrentLinkedQueue#poll to get the elements, which gets them from the beginning.
          Hide
          Colin Patrick McCabe added a comment -

          update: there are a bunch of things in DFSConfigKeys not in hdfs-default.xml. I created HDFS-5656 for this, since it's a change we'd want to do and quickly merge to branch-2.3, etc, and also because it can be decoupled from this JIRA.

          Show
          Colin Patrick McCabe added a comment - update: there are a bunch of things in DFSConfigKeys not in hdfs-default.xml. I created HDFS-5656 for this, since it's a change we'd want to do and quickly merge to branch-2.3, etc, and also because it can be decoupled from this JIRA.
          Hide
          Colin Patrick McCabe added a comment -

          Do we mean to setCachingStrategy in DFSInputStream#getBlockReader? Also, I get that there are a zillion parameters for the BRL constructor, but builders are for when there are optional arguments. Here, it looks like we want to set all of them.

          Actually, in the tests, we often don't set a lot of the arguments. For example, the unit tests don't use the FISCache, may not set readahead, etc. Also, I think there's value in naming the arguments, since otherwise updating the callsites gets very, very difficult.

          We have both verifyChecksum and skipChecksum right now. Let's get rid of one, seems error-prone to be flipping booleans.

          OK. I updated to BlockReaderFactory#newShortCircuitBlockReader to use skipChecksums as well.

          A little note on the history here: prior to the introduction of mlock, it was more straightforward to have a simple positive boolean "verifyChecksum" than to have the skip boolean. But now that we have mlock, verifyChecksum = true might be a lie, since mlock might mean we don't verify.

          The skipChecksum || mlocked.get() idiom is used in a few places, maybe should be a shouldSkipChecksum() method?

          OK.

          IIUC, fillDataBuf fills the bounce buffer, and drainBounceBuffer empties it. Rename fillDataBuf to fillBounceBuffer for parity?

          I renamed drainBounceBuffer to drainDataBuf for symmetry.

          I'm wondering what happens in the bounce buffer read paths when readahead is turned off. It looks like they use readahead to determine how much to read, regardless of the bytes needed, so what if it's zero?

          We always buffer at least a single chunk, even if readahead is turned off. The mechanics of checksumming require this.

          For the slow lane, fillDataBuf doesn't actually fill the returned buf, so when we hit the EOF and break, it looks like we make the user read again to flush out the bounce buffer. Can we save this?

          Yeah, the current code could result in us doing an extra pread even after we know we're at EOF. Let me see if I can avoid that.

          fillDataBuf doesn't fill just the data buf, it also fills the checksum buf and verifies checksums via fillBuffer. Would be nice to javadoc this.

          OK

          I noticed there are two readahead config options too, dfs.client.cache.readahead and dfs.datanode.readahead.bytes. Seems like we should try to emulate the same behavior as remote reads which (according to hdfs-default.xml) use the DN setting, and override with the client setting. Right now it's just using the DN readahead in BRL, so the test that sets client readahead to 0 isn't doing much.

          Right now, the readahead is coming out of DFSClient#cachingStrategy, so it will be coming from dfs.client.cache.readahead, unless someone has overridden it for that DFSInputStream object. The problem with defaulting to the DN setting, is that we don't know what that is (we're on the client, not the DN).

          I don't quite understand why we check needed > maxReadahead... for the fast lane. Once we're checksum aligned via draining the bounce buffer, can't we just stay in the fast lane? Seems like the slow vs. fast lane determination should be based on read alignment, not bytes left.

          The issue is that we want to honor the readahead setting. We would not be doing this if we did a shorter read directly into the provided buffer.

          It's a little weird to me that the readahead chunks is min'd with the buffer size (default 1MB). I get why (memory consumption), but this linkage should be documented somewhere.

          I added a comment.

          DirectBufferPool, would it be better to use a Deque's stack operations rather than a Queue? Might give better cache locality to do LIFO rather than FIFO.

          Interesting point. I will try that and see what numbers I get.

          TestEnhancedByteBufferAccess has an import only change

          OK. I will avoid doing that to make merging easier.

          Kinda unrelated, but should the "dfs.client.read.shortcircuit.*" keys be in hdfs-default.xml? I also noticed that "dfs.client.cache.readahead" says "this setting causes the datanode to..." so the readahead documentation might need to be updated too.

          I'll update it with the information about short-circuit

          Show
          Colin Patrick McCabe added a comment - Do we mean to setCachingStrategy in DFSInputStream#getBlockReader? Also, I get that there are a zillion parameters for the BRL constructor, but builders are for when there are optional arguments. Here, it looks like we want to set all of them. Actually, in the tests, we often don't set a lot of the arguments. For example, the unit tests don't use the FISCache, may not set readahead, etc. Also, I think there's value in naming the arguments, since otherwise updating the callsites gets very, very difficult. We have both verifyChecksum and skipChecksum right now. Let's get rid of one, seems error-prone to be flipping booleans. OK. I updated to BlockReaderFactory#newShortCircuitBlockReader to use skipChecksums as well. A little note on the history here: prior to the introduction of mlock, it was more straightforward to have a simple positive boolean "verifyChecksum" than to have the skip boolean. But now that we have mlock, verifyChecksum = true might be a lie, since mlock might mean we don't verify. The skipChecksum || mlocked.get() idiom is used in a few places, maybe should be a shouldSkipChecksum() method? OK. IIUC, fillDataBuf fills the bounce buffer, and drainBounceBuffer empties it. Rename fillDataBuf to fillBounceBuffer for parity? I renamed drainBounceBuffer to drainDataBuf for symmetry. I'm wondering what happens in the bounce buffer read paths when readahead is turned off. It looks like they use readahead to determine how much to read, regardless of the bytes needed, so what if it's zero? We always buffer at least a single chunk, even if readahead is turned off. The mechanics of checksumming require this. For the slow lane, fillDataBuf doesn't actually fill the returned buf, so when we hit the EOF and break, it looks like we make the user read again to flush out the bounce buffer. Can we save this? Yeah, the current code could result in us doing an extra pread even after we know we're at EOF. Let me see if I can avoid that. fillDataBuf doesn't fill just the data buf, it also fills the checksum buf and verifies checksums via fillBuffer. Would be nice to javadoc this. OK I noticed there are two readahead config options too, dfs.client.cache.readahead and dfs.datanode.readahead.bytes. Seems like we should try to emulate the same behavior as remote reads which (according to hdfs-default.xml) use the DN setting, and override with the client setting. Right now it's just using the DN readahead in BRL, so the test that sets client readahead to 0 isn't doing much. Right now, the readahead is coming out of DFSClient#cachingStrategy , so it will be coming from dfs.client.cache.readahead , unless someone has overridden it for that DFSInputStream object. The problem with defaulting to the DN setting, is that we don't know what that is (we're on the client, not the DN). I don't quite understand why we check needed > maxReadahead... for the fast lane. Once we're checksum aligned via draining the bounce buffer, can't we just stay in the fast lane? Seems like the slow vs. fast lane determination should be based on read alignment, not bytes left. The issue is that we want to honor the readahead setting. We would not be doing this if we did a shorter read directly into the provided buffer. It's a little weird to me that the readahead chunks is min'd with the buffer size (default 1MB). I get why (memory consumption), but this linkage should be documented somewhere. I added a comment. DirectBufferPool, would it be better to use a Deque's stack operations rather than a Queue? Might give better cache locality to do LIFO rather than FIFO. Interesting point. I will try that and see what numbers I get. TestEnhancedByteBufferAccess has an import only change OK. I will avoid doing that to make merging easier. Kinda unrelated, but should the "dfs.client.read.shortcircuit.*" keys be in hdfs-default.xml? I also noticed that "dfs.client.cache.readahead" says "this setting causes the datanode to..." so the readahead documentation might need to be updated too. I'll update it with the information about short-circuit
          Hide
          Andrew Wang added a comment -

          This is good stuff, thanks Colin. I have a few review comments:

          BRL-related:

          • Do we mean to setCachingStrategy in DFSInputStream#getBlockReader? Also, I get that there are a zillion parameters for the BRL constructor, but builders are for when there are optional arguments. Here, it looks like we want to set all of them.
          • We have both verifyChecksum and skipChecksum right now. Let's get rid of one, seems error-prone to be flipping booleans.
          • The skipChecksum || mlocked.get() idiom is used in a few places, maybe should be a shouldSkipChecksum() method?
          • IIUC, fillDataBuf fills the bounce buffer, and drainBounceBuffer empties it. Rename fillDataBuf to fillBounceBuffer for parity?
          • BlockReaderLocal:500, trailing whitespace
          • I'm wondering what happens in the bounce buffer read paths when readahead is turned off. It looks like they use readahead to determine how much to read, regardless of the bytes needed, so what if it's zero?
          • For the slow lane, fillDataBuf doesn't actually fill the returned buf, so when we hit the EOF and break, it looks like we make the user read again to flush out the bounce buffer. Can we save this?
          • fillDataBuf doesn't fill just the data buf, it also fills the checksum buf and verifies checksums via fillBuffer. Would be nice to javadoc this.
          • I noticed there are two readahead config options too, dfs.client.cache.readahead and dfs.datanode.readahead.bytes. Seems like we should try to emulate the same behavior as remote reads which (according to hdfs-default.xml) use the DN setting, and override with the client setting. Right now it's just using the DN readahead in BRL, so the test that sets client readahead to 0 isn't doing much.
          • I don't quite understand why we check needed > maxReadahead... for the fast lane. Once we're checksum aligned via draining the bounce buffer, can't we just stay in the fast lane? Seems like the slow vs. fast lane determination should be based on read alignment, not bytes left.

          Little stuff:

          • It's a little weird to me that the readahead chunks is min'd with the buffer size (default 1MB). I get why (memory consumption), but this linkage should be documented somewhere.
          • DirectBufferPool, would it be better to use a Deque's stack operations rather than a Queue? Might give better cache locality to do LIFO rather than FIFO.
          • TestEnhancedByteBufferAccess has an import only change
          • Kinda unrelated, but should the "dfs.client.read.shortcircuit.*" keys be in hdfs-default.xml? I also noticed that "dfs.client.cache.readahead" says "this setting causes the datanode to..." so the readahead documentation might need to be updated too.
          Show
          Andrew Wang added a comment - This is good stuff, thanks Colin. I have a few review comments: BRL-related: Do we mean to setCachingStrategy in DFSInputStream#getBlockReader? Also, I get that there are a zillion parameters for the BRL constructor, but builders are for when there are optional arguments. Here, it looks like we want to set all of them. We have both verifyChecksum and skipChecksum right now. Let's get rid of one, seems error-prone to be flipping booleans. The skipChecksum || mlocked.get() idiom is used in a few places, maybe should be a shouldSkipChecksum() method? IIUC, fillDataBuf fills the bounce buffer, and drainBounceBuffer empties it. Rename fillDataBuf to fillBounceBuffer for parity? BlockReaderLocal:500, trailing whitespace I'm wondering what happens in the bounce buffer read paths when readahead is turned off. It looks like they use readahead to determine how much to read, regardless of the bytes needed, so what if it's zero? For the slow lane, fillDataBuf doesn't actually fill the returned buf, so when we hit the EOF and break, it looks like we make the user read again to flush out the bounce buffer. Can we save this? fillDataBuf doesn't fill just the data buf, it also fills the checksum buf and verifies checksums via fillBuffer . Would be nice to javadoc this. I noticed there are two readahead config options too, dfs.client.cache.readahead and dfs.datanode.readahead.bytes. Seems like we should try to emulate the same behavior as remote reads which (according to hdfs-default.xml) use the DN setting, and override with the client setting. Right now it's just using the DN readahead in BRL, so the test that sets client readahead to 0 isn't doing much. I don't quite understand why we check needed > maxReadahead... for the fast lane. Once we're checksum aligned via draining the bounce buffer, can't we just stay in the fast lane? Seems like the slow vs. fast lane determination should be based on read alignment, not bytes left. Little stuff: It's a little weird to me that the readahead chunks is min'd with the buffer size (default 1MB). I get why (memory consumption), but this linkage should be documented somewhere. DirectBufferPool, would it be better to use a Deque's stack operations rather than a Queue? Might give better cache locality to do LIFO rather than FIFO. TestEnhancedByteBufferAccess has an import only change Kinda unrelated, but should the "dfs.client.read.shortcircuit.*" keys be in hdfs-default.xml? I also noticed that "dfs.client.cache.readahead" says "this setting causes the datanode to..." so the readahead documentation might need to be updated too.
          Hide
          Hadoop QA added a comment -

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

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

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

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

          +1 javadoc. The javadoc tool did not generate any 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/5655//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5655//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/12617331/HDFS-5634.002.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 4 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any 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/5655//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5655//console This message is automatically generated.
          Hide
          Colin Patrick McCabe added a comment -
          • fix findbugs warnings
          • fix bug in BlockReaderLocal#skip
          • fix bug causing mmaps to be granted even without checksum-skipping in effect
          Show
          Colin Patrick McCabe added a comment - fix findbugs warnings fix bug in BlockReaderLocal#skip fix bug causing mmaps to be granted even without checksum-skipping in effect
          Hide
          Hadoop QA added a comment -

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

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

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

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

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

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

          -1 findbugs. The patch appears to introduce 2 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 in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.fs.TestEnhancedByteBufferAccess

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5648//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/5648//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5648//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/12617232/HDFS-5634.001.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. -1 findbugs . The patch appears to introduce 2 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 in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.fs.TestEnhancedByteBufferAccess +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5648//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/5648//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5648//console This message is automatically generated.
          Hide
          Colin Patrick McCabe added a comment -

          This change allows the BlockReaderLocal to switch back and forth between mlocked and non-mlocked states. (Later, we will hook up callbacks from code added by HDFS-5182 to do this).

          A few other changes:

          • honor the readahead parameter, so that skipping checksums doesn't mean skipping all buffering. See HDFS-4710 of the discussion of why we want this.
          • for reads to a direct ByteBuffer, add a "fast lane" that copies directly into the user-supplied ByteBuffer. We only do this if the read is longer than our configured readahead. This avoids a copy.
          • use pread everywhere instead of read. This means that if a client opens a file multiple times, they only need one set of file descriptors rather than multiple. This will become more important with HDFS-5182, since that change will add a notification system per set of FDs. We don't want to track too many of those.
          • move reading of the meta file header out of the BlockReaderLocal constructor. This will allow us to implement HDFS-4960 (only read version once). This is mainly a win in the no-checksum case.
          • avoid using a skip buffer in BlockReaderLocal#skip (implements HDFS-5574)
          Show
          Colin Patrick McCabe added a comment - This change allows the BlockReaderLocal to switch back and forth between mlocked and non-mlocked states. (Later, we will hook up callbacks from code added by HDFS-5182 to do this). A few other changes: honor the readahead parameter, so that skipping checksums doesn't mean skipping all buffering. See HDFS-4710 of the discussion of why we want this. for reads to a direct ByteBuffer, add a "fast lane" that copies directly into the user-supplied ByteBuffer. We only do this if the read is longer than our configured readahead. This avoids a copy. use pread everywhere instead of read. This means that if a client opens a file multiple times, they only need one set of file descriptors rather than multiple. This will become more important with HDFS-5182 , since that change will add a notification system per set of FDs. We don't want to track too many of those. move reading of the meta file header out of the BlockReaderLocal constructor. This will allow us to implement HDFS-4960 (only read version once). This is mainly a win in the no-checksum case. avoid using a skip buffer in BlockReaderLocal#skip (implements HDFS-5574 )

            People

            • Assignee:
              Colin Patrick McCabe
              Reporter:
              Colin Patrick McCabe
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development