Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-3110

libhdfs implementation of direct read API

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0.2-alpha
    • Component/s: libhdfs, performance
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      libhdfs is enhanced to read directly into user-supplied buffers when possible, reducing the number of memory copies.

      Description

      Once HDFS-2834 gets committed, we can add support for the new API to libhdfs, which leads to significant performance increases when reading local data from C.

      1. HDFS-3110.0.patch
        4 kB
        Henry Robinson
      2. HDFS-3110.1.patch
        13 kB
        Henry Robinson
      3. HDFS-3110.2.patch
        20 kB
        Henry Robinson
      4. HDFS-3110.3.patch
        19 kB
        Henry Robinson
      5. HDFS-3110.4.patch
        20 kB
        Henry Robinson
      6. HDFS-3110.5.patch
        20 kB
        Henry Robinson

        Issue Links

          Activity

          Hide
          Todd Lipcon added a comment -

          Committed to branch-2 as well.

          Show
          Todd Lipcon added a comment - Committed to branch-2 as well.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1041 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1041/)
          HDFS-3110. Use directRead API to reduce the number of buffer copies in libhdfs. Contributed by Henry Robinson. (Revision 1310185)

          Result = SUCCESS
          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1310185
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.h
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs_test.c
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/tests/test-libhdfs.sh
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1041 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1041/ ) HDFS-3110 . Use directRead API to reduce the number of buffer copies in libhdfs. Contributed by Henry Robinson. (Revision 1310185) Result = SUCCESS todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1310185 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.h /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs_test.c /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/tests/test-libhdfs.sh
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1006 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1006/)
          HDFS-3110. Use directRead API to reduce the number of buffer copies in libhdfs. Contributed by Henry Robinson. (Revision 1310185)

          Result = FAILURE
          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1310185
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.h
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs_test.c
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/tests/test-libhdfs.sh
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1006 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1006/ ) HDFS-3110 . Use directRead API to reduce the number of buffer copies in libhdfs. Contributed by Henry Robinson. (Revision 1310185) Result = FAILURE todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1310185 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.h /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs_test.c /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/tests/test-libhdfs.sh
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #2027 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2027/)
          HDFS-3110. Use directRead API to reduce the number of buffer copies in libhdfs. Contributed by Henry Robinson. (Revision 1310185)

          Result = ABORTED
          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1310185
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.h
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs_test.c
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/tests/test-libhdfs.sh
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #2027 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2027/ ) HDFS-3110 . Use directRead API to reduce the number of buffer copies in libhdfs. Contributed by Henry Robinson. (Revision 1310185) Result = ABORTED todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1310185 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.h /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs_test.c /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/tests/test-libhdfs.sh
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #2013 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2013/)
          HDFS-3110. Use directRead API to reduce the number of buffer copies in libhdfs. Contributed by Henry Robinson. (Revision 1310185)

          Result = SUCCESS
          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1310185
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.h
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs_test.c
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/tests/test-libhdfs.sh
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #2013 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2013/ ) HDFS-3110 . Use directRead API to reduce the number of buffer copies in libhdfs. Contributed by Henry Robinson. (Revision 1310185) Result = SUCCESS todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1310185 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.h /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs_test.c /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/tests/test-libhdfs.sh
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #2088 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2088/)
          HDFS-3110. Use directRead API to reduce the number of buffer copies in libhdfs. Contributed by Henry Robinson. (Revision 1310185)

          Result = SUCCESS
          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1310185
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.h
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs_test.c
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/tests/test-libhdfs.sh
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #2088 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2088/ ) HDFS-3110 . Use directRead API to reduce the number of buffer copies in libhdfs. Contributed by Henry Robinson. (Revision 1310185) Result = SUCCESS todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1310185 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.h /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs_test.c /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/tests/test-libhdfs.sh
          Hide
          Todd Lipcon added a comment -

          I was able to run the tests. I got some errors on the write side, but then I verified that those errors were present before this patch as well. So, I don't think it's worth blocking this patch on it, given it makes good progress getting the tests back running at all!

          Thanks for the contribution, Henry! Nice speedup!

          Show
          Todd Lipcon added a comment - I was able to run the tests. I got some errors on the write side, but then I verified that those errors were present before this patch as well. So, I don't think it's worth blocking this patch on it, given it makes good progress getting the tests back running at all! Thanks for the contribution, Henry! Nice speedup!
          Hide
          Henry Robinson added a comment -

          In actual fact, you don't need to set either of those environment variables (I was just c&p'ing from my ad-hoc script). Plus you should move a.out to hdfs_test either from gcc or manually.

          Show
          Henry Robinson added a comment - In actual fact, you don't need to set either of those environment variables (I was just c&p'ing from my ad-hoc script). Plus you should move a.out to hdfs_test either from gcc or manually.
          Hide
          Henry Robinson added a comment -

          Yeah, the tests aren't wired into the pom yet (there is a TODO in hadoop-hdfs/pom.xml). We should create a follow-on JIRA to put this back in the build process.

          Here's how I built. Make sure HADOOP_HOME is set.

          From hadoop-hdfs/src/main/native:

          export LD_LIBRARY_PATH=/usr/lib/jvm/java-6-sun-1.6.0.26/jre/lib/amd64/server:${HADOOP_HOME}/lib/native/
          
          export LIBHDFS_OPTS=-Djava.library.path=${HADOOP_HOME}/lib/native/
          
          gcc -Wall hdfs_test.c -I/usr/lib/jvm/java-6-sun-1.6.0.26/include/ -I/usr/lib/jvm/java-6-sun-1.6.0.26/include/linux -L${HADOOP_HOME}/lib/native/ -lhdfs -ljvm -L/usr/lib/jvm/java-6-sun-1.6.0.26/jre/lib/amd64/server/ -lrt
          

          and to test:

          From hadoop-hdfs/src/main/native/tests

          ./test-libhdfs.sh
          
          Show
          Henry Robinson added a comment - Yeah, the tests aren't wired into the pom yet (there is a TODO in hadoop-hdfs/pom.xml). We should create a follow-on JIRA to put this back in the build process. Here's how I built. Make sure HADOOP_HOME is set. From hadoop-hdfs/src/main/native: export LD_LIBRARY_PATH=/usr/lib/jvm/java-6-sun-1.6.0.26/jre/lib/amd64/server:${HADOOP_HOME}/lib/ native / export LIBHDFS_OPTS=-Djava.library.path=${HADOOP_HOME}/lib/ native / gcc -Wall hdfs_test.c -I/usr/lib/jvm/java-6-sun-1.6.0.26/include/ -I/usr/lib/jvm/java-6-sun-1.6.0.26/include/linux -L${HADOOP_HOME}/lib/ native / -lhdfs -ljvm -L/usr/lib/jvm/java-6-sun-1.6.0.26/jre/lib/amd64/server/ -lrt and to test: From hadoop-hdfs/src/main/native/tests ./test-libhdfs.sh
          Hide
          Todd Lipcon added a comment -

          Hey Henry. The patch looks good, but I can't figure out how to run the test. When I do mvn -Pnative -DskipTests install, it builds libhdfs, but doesn't build the hdfs_test binary. Can you post instructions on how to run the test manually? Then we can do another jira to make it more automatic.

          Show
          Todd Lipcon added a comment - Hey Henry. The patch looks good, but I can't figure out how to run the test. When I do mvn -Pnative -DskipTests install, it builds libhdfs, but doesn't build the hdfs_test binary. Can you post instructions on how to run the test manually? Then we can do another jira to make it more automatic.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 7 new or modified 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 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 .

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2189//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2189//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/12521434/HDFS-3110.5.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 7 new or modified 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 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 . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2189//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2189//console This message is automatically generated.
          Hide
          Henry Robinson added a comment -

          You're right - my original thinking was that checking errno != 0 let us know that we aren't at EOF, but in fact that's captured by checking whether readDirect == 0 (since EOF and 0-byte reads are indistinguishable by return code).

          I've removed the check and reworded the comment

          Show
          Henry Robinson added a comment - You're right - my original thinking was that checking errno != 0 let us know that we aren't at EOF, but in fact that's captured by checking whether readDirect == 0 (since EOF and 0-byte reads are indistinguishable by return code). I've removed the check and reworded the comment
          Hide
          Todd Lipcon added a comment -

          Sorry, missed this in previous rev:

          +              // Unexpected error, but not EOF. Clear error.
          

          is this really supposed to say "but not EOF"? Seems like it should say something like "but not UnsupportedOperationException" no?

          Aside from that, +1.

          Show
          Todd Lipcon added a comment - Sorry, missed this in previous rev: + // Unexpected error, but not EOF. Clear error. is this really supposed to say "but not EOF"? Seems like it should say something like "but not UnsupportedOperationException" no? Aside from that, +1.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 7 new or modified 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 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 .

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2179//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2179//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/12521262/HDFS-3110.4.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 7 new or modified 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 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 . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2179//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2179//console This message is automatically generated.
          Hide
          Henry Robinson added a comment -

          Patch addresses Todd's latest comments.

          Show
          Henry Robinson added a comment - Patch addresses Todd's latest comments.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 7 new or modified 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 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 .

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2174//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2174//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/12521235/HDFS-3110.3.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 7 new or modified 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 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 . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2174//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2174//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          Sorry, a few things I missed when I looked at the earlier rev:

          • we no longer need to expose the hdfsReadDirect API in hdfs.h, given it's automatically handled. Instead we should probably rename to readDirect and make it static in hdfs.c. If you need it for the test case, then I think we should just add it as an extern declaration in the test, and leave it non-static – but not advertise it in hdfs.h. We don't want users to think they have to use it explicitly.

          +    // Bit fields for  hdfsFile_internal flags
          

          nit: extra space


          Looks like handleReadResult returns -1 when the underlying code calls an exception. Then we have the following code:

          +    if (noReadBytes != 0) {
          +      (*env)->GetByteArrayRegion(env, jbRarray, 0, noReadBytes, buffer);
               }
          

          won't that end up calling GetByteArrayRegion with a negative length? Seems suspicious.

          Show
          Todd Lipcon added a comment - Sorry, a few things I missed when I looked at the earlier rev: we no longer need to expose the hdfsReadDirect API in hdfs.h, given it's automatically handled. Instead we should probably rename to readDirect and make it static in hdfs.c. If you need it for the test case, then I think we should just add it as an extern declaration in the test, and leave it non-static – but not advertise it in hdfs.h. We don't want users to think they have to use it explicitly. + // Bit fields for hdfsFile_internal flags nit: extra space Looks like handleReadResult returns -1 when the underlying code calls an exception. Then we have the following code: + if (noReadBytes != 0) { + (*env)->GetByteArrayRegion(env, jbRarray, 0, noReadBytes, buffer); } won't that end up calling GetByteArrayRegion with a negative length? Seems suspicious.
          Hide
          Henry Robinson added a comment -

          New patch that's actually a diff vs trunk this time :/

          I incorporated most of Todd's suggestions. I've left HDFS_FILE_SUPPORTS_DIRECT_READ in hdfs.h for now so that users who really want to turn off support for some reason (perhaps a bug) have access to the flag that they can set in hdfsFile's guts.

          I ran the tests against the default local filesystem when no fs.default.name is set, and observed no errors except that the tests expect readDirect to be available.

          Show
          Henry Robinson added a comment - New patch that's actually a diff vs trunk this time :/ I incorporated most of Todd's suggestions. I've left HDFS_FILE_SUPPORTS_DIRECT_READ in hdfs.h for now so that users who really want to turn off support for some reason (perhaps a bug) have access to the flag that they can set in hdfsFile's guts. I ran the tests against the default local filesystem when no fs.default.name is set, and observed no errors except that the tests expect readDirect to be available.
          Hide
          Todd Lipcon added a comment -

          my top comment got chopped somehow above:

          • I like the refactoring out of readPrepare and handleReadResult. But, these should be declared static
          Show
          Todd Lipcon added a comment - my top comment got chopped somehow above: I like the refactoring out of readPrepare and handleReadResult. But, these should be declared static
          Hide
          Todd Lipcon added a comment -

          uld be declared static

          • I think your new patch was actually a delta vs the old patch, instead of a completely new one vs trunk. We need a new one for QA & commit
          • When NewDirectByteBuffer returns NULL with no errno set, I think it's better to set errno = ENOMEM; in an else clause – just a little easier to read.
          • The new flag HDFS_SUPPORTS_DIRECT_READ is only used internally, so not sure it belongs in the public header hdfs.h (this is what users include, right?). Also, I think it would be better named something like HDFS_FILE_SUPPORTS_DIRECT_READ since it refers to a specific stream rather than the entire FS.
          • Rather than declaring it as a const I think it's better to use an enum or #define, since consts are a C++ thing and this code is mostly straight C. Also, I think it's better to define it as (1 << 0) to indicate that this is going to be in a bitfield.
          • Please add a comment above the definition of the new flag referring to hdfsFile_internal.flags, so we know where the flags end up.
          • the new flags field should be unsigned – uint32_t probably
          • in the new test, why are you hardcoding localhost:20300? I'd think using default as before is the right choice, since it will pick up whatever is fs.default.name in your core-site.xml on the classpath. That way this same test can be run against local FS or against DFS
          Show
          Todd Lipcon added a comment - uld be declared static I think your new patch was actually a delta vs the old patch, instead of a completely new one vs trunk. We need a new one for QA & commit When NewDirectByteBuffer returns NULL with no errno set, I think it's better to set errno = ENOMEM; in an else clause – just a little easier to read. The new flag HDFS_SUPPORTS_DIRECT_READ is only used internally, so not sure it belongs in the public header hdfs.h (this is what users include, right?). Also, I think it would be better named something like HDFS_FILE_SUPPORTS_DIRECT_READ since it refers to a specific stream rather than the entire FS. Rather than declaring it as a const I think it's better to use an enum or #define, since consts are a C++ thing and this code is mostly straight C. Also, I think it's better to define it as (1 << 0) to indicate that this is going to be in a bitfield. Please add a comment above the definition of the new flag referring to hdfsFile_internal.flags, so we know where the flags end up. the new flags field should be unsigned – uint32_t probably in the new test, why are you hardcoding localhost:20300 ? I'd think using default as before is the right choice, since it will pick up whatever is fs.default.name in your core-site.xml on the classpath. That way this same test can be run against local FS or against DFS
          Hide
          Hadoop QA added a comment -

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

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

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

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

          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2170//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/12521180/HDFS-3110.2.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 7 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2170//console This message is automatically generated.
          Hide
          Henry Robinson added a comment -

          Patch addressing Todd's concerns.

          I added a 'flags' field to hdfsFile that has a bit set if a direct read is supported. I detect that by trying to issue a 0-byte read when the file is created. If an exception is thrown, the flag is cleared, otherwise it is set. Once the flag is set, all subsequent hdfsRead calls will be diverted to hdfsReadDirect.

          An alternative is to use reflection to grab the input stream inside FsDataInputStream and use reflection to look for ByteBufferReadable, but that feels a little fragile (and complex to do in C); plus if some FS implements read(ByteBuffer) only to stub it out with a UnsupportedOperationException or similar, reads would never work correctly.

          Show
          Henry Robinson added a comment - Patch addressing Todd's concerns. I added a 'flags' field to hdfsFile that has a bit set if a direct read is supported. I detect that by trying to issue a 0-byte read when the file is created. If an exception is thrown, the flag is cleared, otherwise it is set. Once the flag is set, all subsequent hdfsRead calls will be diverted to hdfsReadDirect. An alternative is to use reflection to grab the input stream inside FsDataInputStream and use reflection to look for ByteBufferReadable, but that feels a little fragile (and complex to do in C); plus if some FS implements read(ByteBuffer) only to stub it out with a UnsupportedOperationException or similar, reads would never work correctly.
          Hide
          Todd Lipcon added a comment -
          +      if ((*env)->ExceptionCheck(env)) {
          +        errno = errnoFromException(NULL, env, "JNIEnv::NewDirectByteBuffer");
          +      }
          

          From my reading of the JNI spec, I think there should always be an exception pending if it returns NULL. But, just in case, I think you should still set errno, so that the contract for the user of this function is that errno is always set if it returns -1. Maybe ENOMEM?


          errnoFromException: add an ENOTSUP case for UnsupportedOperationException, since that seems quite likely


          For "hot code path" functions like read, it may be a good optimization to use the JNI calls directly, with cached methodids, rather than using the hdfsJniHelper functions. They seem to be reasonably costly. But that's a separate optimization, so let's do it in another JIRA.


          +          fprintf(stderr,
          +                  "WARN: FSDataInputStream.read returned invalid return code - libhdfs returning EOF, i.e., 0: %d\n",
          +                  noReadBytes);
          

          Please reformat to 80 columns


          +        //This is a valid case: there aren't any bytes left to read!
          

          I think this would be better as: "Returning -1 is valid – it indicates EOF". And then change the if condition to noReadyBytes != -1


          Is there some way to change the code to automatically use hdfsReadDirect when the underlying stream implements it? For example, when the file is opened, check whether it implements the interface, and if so, stick a flag in the hdfsFile_internal struct. Or, if the interface isn't enough (because of wrapped streams), have it try direct read on the first call to read, and then set a flag and fallback to the slower read call if it gets UnsupportedOperationException?

          I'm thinking that this is an implementation change, not an interface change, so making users know about the new API is wrong.


          Looks like the test shell script has a couple of hard tabs, please use two-space indentation

          Show
          Todd Lipcon added a comment - + if ((*env)->ExceptionCheck(env)) { + errno = errnoFromException(NULL, env, "JNIEnv::NewDirectByteBuffer" ); + } From my reading of the JNI spec, I think there should always be an exception pending if it returns NULL. But, just in case, I think you should still set errno, so that the contract for the user of this function is that errno is always set if it returns -1. Maybe ENOMEM? errnoFromException: add an ENOTSUP case for UnsupportedOperationException, since that seems quite likely For "hot code path" functions like read, it may be a good optimization to use the JNI calls directly, with cached methodids, rather than using the hdfsJniHelper functions. They seem to be reasonably costly. But that's a separate optimization, so let's do it in another JIRA. + fprintf(stderr, + "WARN: FSDataInputStream.read returned invalid return code - libhdfs returning EOF, i.e., 0: %d\n" , + noReadBytes); Please reformat to 80 columns + //This is a valid case : there aren't any bytes left to read! I think this would be better as: "Returning -1 is valid – it indicates EOF". And then change the if condition to noReadyBytes != -1 Is there some way to change the code to automatically use hdfsReadDirect when the underlying stream implements it? For example, when the file is opened, check whether it implements the interface, and if so, stick a flag in the hdfsFile_internal struct. Or, if the interface isn't enough (because of wrapped streams), have it try direct read on the first call to read, and then set a flag and fallback to the slower read call if it gets UnsupportedOperationException? I'm thinking that this is an implementation change, not an interface change, so making users know about the new API is wrong. Looks like the test shell script has a couple of hard tabs, please use two-space indentation
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 6 new or modified 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 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 .

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2151//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2151//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/12520868/HDFS-3110.1.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified 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 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 . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2151//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2151//console This message is automatically generated.
          Hide
          Henry Robinson added a comment -

          Here's a patch with a simple smoke test. I have also reworked test-libhdfs.sh to work against trunk, since it had not been updated since the days of the ant build.

          After setting HADOOP_HOME, running test-libhdfs.sh spins up a mini cluster (from HDFS-3167) and runs the tests correctly for me from a checkout of trunk. This is a better situation than before, even though the script is not yet integrated with the automatic build (there is a todo in the hdfs pom.xml for this).

          Let me know if I should split the refactor of the test script up from this JIRA.

          Show
          Henry Robinson added a comment - Here's a patch with a simple smoke test. I have also reworked test-libhdfs.sh to work against trunk, since it had not been updated since the days of the ant build. After setting HADOOP_HOME, running test-libhdfs.sh spins up a mini cluster (from HDFS-3167 ) and runs the tests correctly for me from a checkout of trunk. This is a better situation than before, even though the script is not yet integrated with the automatic build (there is a todo in the hdfs pom.xml for this). Let me know if I should split the refactor of the test script up from this JIRA.
          Hide
          Henry Robinson added a comment -

          Fyi, I've added tests, but when I run with the libhdfs test script I get instances of ChecksumFileSystem back which are no good for this case because they don't support the read(ByteBuffer) interface. So I've added a class to HDFS-3167 that allows us to spin up a simple cluster for correctness testing very easily, and if that goes in I'll be able to update the test script accordingly.

          Show
          Henry Robinson added a comment - Fyi, I've added tests, but when I run with the libhdfs test script I get instances of ChecksumFileSystem back which are no good for this case because they don't support the read(ByteBuffer) interface. So I've added a class to HDFS-3167 that allows us to spin up a simple cluster for correctness testing very easily, and if that goes in I'll be able to update the test script accordingly.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12519573/HDFS-3110.0.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 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 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 .

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2080//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2080//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/12519573/HDFS-3110.0.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 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 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 . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2080//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2080//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          Hey Henry. Can you add some tests to hdfs_test.c?

          Show
          Todd Lipcon added a comment - Hey Henry. Can you add some tests to hdfs_test.c?
          Hide
          Henry Robinson added a comment -

          Patch against trunk taking into account Todd's comments when this was initially part of HDFS-2834

          Show
          Henry Robinson added a comment - Patch against trunk taking into account Todd's comments when this was initially part of HDFS-2834

            People

            • Assignee:
              Henry Robinson
              Reporter:
              Henry Robinson
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development