Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.23.0
    • Fix Version/s: 1.1.0, 0.23.0
    • Component/s: io, native, performance
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Target Version/s:

      Description

      This JIRA adds JNI wrappers for sync_data_range and posix_fadvise. It also implements a ReadaheadPool class for future use from HDFS and MapReduce.

      1. hadoop-7753.txt
        19 kB
        Todd Lipcon
      2. hadoop-7753.txt
        19 kB
        Todd Lipcon
      3. hadoop-7753.txt
        19 kB
        Todd Lipcon
      4. hadoop-7753.txt
        20 kB
        Todd Lipcon
      5. hadoop-7753.txt
        20 kB
        Todd Lipcon
      6. HADOOP-7753.branch-1.patch
        20 kB
        Brandon Li
      7. HADOOP-7753.branch-1.patch
        20 kB
        Brandon Li

        Issue Links

          Activity

          Hide
          Suresh Srinivas added a comment -

          I committed this patch to branch-1.

          Show
          Suresh Srinivas added a comment - I committed this patch to branch-1.
          Hide
          Cristina L. Abad added a comment -

          Yes, I know that. The issue is that some people may be using the 32-bit libraries on 64-bit architectures. This actually works fine except if you turn on these features and you are running on a system with the bug I mentioned. Anyway, this is a minor problem, just thought it would be a good idea to mention it here in case someone runs into the problem and doesn't know what's going on.

          Show
          Cristina L. Abad added a comment - Yes, I know that. The issue is that some people may be using the 32-bit libraries on 64-bit architectures. This actually works fine except if you turn on these features and you are running on a system with the bug I mentioned. Anyway, this is a minor problem, just thought it would be a good idea to mention it here in case someone runs into the problem and doesn't know what's going on.
          Hide
          Giridharan Kesavan added a comment -

          All the hadoop releases ships both 32 and 64 bit libraries. User can decide on using 32 or 64 bit library which is appropriate for his environment. So the ans is we compile 32 and 64 bit libs by setting CFLAGS and CXXFLAGS with the appropriate JDK - for more info refer to hadoop release wiki.

          Show
          Giridharan Kesavan added a comment - All the hadoop releases ships both 32 and 64 bit libraries. User can decide on using 32 or 64 bit library which is appropriate for his environment. So the ans is we compile 32 and 64 bit libs by setting CFLAGS and CXXFLAGS with the appropriate JDK - for more info refer to hadoop release wiki.
          Hide
          Cristina L. Abad added a comment -

          In NativeIO.c it might be worth referencing RedHat BZ 554735 (see https://bugzilla.redhat.com/show_bug.cgi?id=554735). We were doing some tests on RHEL5.4 and that bug lead to exceptions (and decreased performance). We fixed it by compiling/using the 64-bit native libraries. We are now seeing significant increase in performance: an average of 24% improvement in running time across 10-runs of the Sort benchmark (10-node cluster, 35GB being sorted).

          Show
          Cristina L. Abad added a comment - In NativeIO.c it might be worth referencing RedHat BZ 554735 (see https://bugzilla.redhat.com/show_bug.cgi?id=554735 ). We were doing some tests on RHEL5.4 and that bug lead to exceptions (and decreased performance). We fixed it by compiling/using the 64-bit native libraries. We are now seeing significant increase in performance: an average of 24% improvement in running time across 10-runs of the Sort benchmark (10-node cluster, 35GB being sorted).
          Hide
          Brandon Li added a comment -

          Thanks. I ran teragen/terasort/teravalidate/TestDFSIO tests along with the included unit test in the patch. The other half of the fadvise support is in HDFS-2465. I've also uploaded a patch there.

          Show
          Brandon Li added a comment - Thanks. I ran teragen/terasort/teravalidate/TestDFSIO tests along with the included unit test in the patch. The other half of the fadvise support is in HDFS-2465 . I've also uploaded a patch there.
          Hide
          Suresh Srinivas added a comment -

          Brandon, +1 for the change. What tests did you run?

          Show
          Suresh Srinivas added a comment - Brandon, +1 for the change. What tests did you run?
          Hide
          Brandon Li added a comment -

          Removed guava dependency from the patch for branch-1.

          Show
          Brandon Li added a comment - Removed guava dependency from the patch for branch-1.
          Hide
          Kihwal Lee added a comment -

          btw, which jar pulled by webhdfs into Hadoop 1.x caused problems? we should try to fix that too.

          We've seen users having trouble with jersey, which was brought in by webhdfs. For 1.x it is already broken, like Eli said. We tend to refrain from introducing new dependencies and be conservative on upping versions. Since users have adapted to the current broken status, changing (add/remove/upgrade) them causes a lot of pain.

          Show
          Kihwal Lee added a comment - btw, which jar pulled by webhdfs into Hadoop 1.x caused problems? we should try to fix that too. We've seen users having trouble with jersey, which was brought in by webhdfs. For 1.x it is already broken, like Eli said. We tend to refrain from introducing new dependencies and be conservative on upping versions. Since users have adapted to the current broken status, changing (add/remove/upgrade) them causes a lot of pain.
          Hide
          Brandon Li added a comment -

          Let me try to replace the used guava code to removed the guava library dependency.
          btw, which jar pulled by webhdfs into Hadoop 1.x caused problems? we should try to fix that too.

          Show
          Brandon Li added a comment - Let me try to replace the used guava code to removed the guava library dependency. btw, which jar pulled by webhdfs into Hadoop 1.x caused problems? we should try to fix that too.
          Hide
          Eli Collins added a comment -

          In CDH we jarjar the guava jar and use/depend on that. Hadoop 1.x is already pretty broken in this respect as webhdfs and other changes have pulled in a lot of jars which may already be on downstream or user classpaths so it may not be worth addressing here.

          Show
          Eli Collins added a comment - In CDH we jarjar the guava jar and use/depend on that. Hadoop 1.x is already pretty broken in this respect as webhdfs and other changes have pulled in a lot of jars which may already be on downstream or user classpaths so it may not be worth addressing here.
          Hide
          Brandon Li added a comment -

          Note that adding a new library like guava here will break users/downstream components who don't expect guava to be on their classpath, eg because they're already using a different version of guava.

          Yes. Makes sense. what's your suggestion to work around it?

          Show
          Brandon Li added a comment - Note that adding a new library like guava here will break users/downstream components who don't expect guava to be on their classpath, eg because they're already using a different version of guava. Yes. Makes sense. what's your suggestion to work around it?
          Hide
          Eli Collins added a comment -

          Note that adding a new library like guava here will break users/downstream components who don't expect guava to be on their classpath, eg because they're already using a different version of guava.

          Show
          Eli Collins added a comment - Note that adding a new library like guava here will break users/downstream components who don't expect guava to be on their classpath, eg because they're already using a different version of guava.
          Hide
          Brandon Li added a comment -

          Back port the patch and add Guava library to branch-1.

          Show
          Brandon Li added a comment - Back port the patch and add Guava library to branch-1.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #846 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/846/)
          HADOOP-7753. Support fadvise and sync_file_range in NativeIO. Add ReadaheadPool infrastructure for use in HDFS and MR. Contributed by Todd Lipcon.

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

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/ReadaheadPool.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/configure.ac
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/file_descriptor.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/nativeio/TestNativeIO.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #846 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/846/ ) HADOOP-7753 . Support fadvise and sync_file_range in NativeIO. Add ReadaheadPool infrastructure for use in HDFS and MR. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190067 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/ReadaheadPool.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/configure.ac /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/file_descriptor.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/nativeio/TestNativeIO.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-0.23-Build #53 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/53/)
          HADOOP-7753. Support fadvise and sync_file_range in NativeIO. Add ReadaheadPool infrastructure for use in HDFS and MR. Contributed by Todd Lipcon.

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

          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/ReadaheadPool.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/configure.ac
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/file_descriptor.c
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/nativeio/TestNativeIO.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-0.23-Build #53 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/53/ ) HADOOP-7753 . Support fadvise and sync_file_range in NativeIO. Add ReadaheadPool infrastructure for use in HDFS and MR. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190066 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/ReadaheadPool.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/configure.ac /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/file_descriptor.c /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/nativeio/TestNativeIO.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-0.23-Build #65 (See https://builds.apache.org/job/Hadoop-Mapreduce-0.23-Build/65/)
          HADOOP-7753. Support fadvise and sync_file_range in NativeIO. Add ReadaheadPool infrastructure for use in HDFS and MR. Contributed by Todd Lipcon.

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

          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/ReadaheadPool.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/configure.ac
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/file_descriptor.c
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/nativeio/TestNativeIO.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-0.23-Build #65 (See https://builds.apache.org/job/Hadoop-Mapreduce-0.23-Build/65/ ) HADOOP-7753 . Support fadvise and sync_file_range in NativeIO. Add ReadaheadPool infrastructure for use in HDFS and MR. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190066 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/ReadaheadPool.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/configure.ac /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/file_descriptor.c /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/nativeio/TestNativeIO.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #874 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/874/)
          HADOOP-7753. Support fadvise and sync_file_range in NativeIO. Add ReadaheadPool infrastructure for use in HDFS and MR. Contributed by Todd Lipcon.

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

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/ReadaheadPool.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/configure.ac
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/file_descriptor.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/nativeio/TestNativeIO.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #874 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/874/ ) HADOOP-7753 . Support fadvise and sync_file_range in NativeIO. Add ReadaheadPool infrastructure for use in HDFS and MR. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190067 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/ReadaheadPool.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/configure.ac /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/file_descriptor.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/nativeio/TestNativeIO.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #1194 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1194/)
          HADOOP-7753. Support fadvise and sync_file_range in NativeIO. Add ReadaheadPool infrastructure for use in HDFS and MR. Contributed by Todd Lipcon.

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

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/ReadaheadPool.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/configure.ac
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/file_descriptor.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/nativeio/TestNativeIO.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #1194 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1194/ ) HADOOP-7753 . Support fadvise and sync_file_range in NativeIO. Add ReadaheadPool infrastructure for use in HDFS and MR. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190067 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/ReadaheadPool.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/configure.ac /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/file_descriptor.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/nativeio/TestNativeIO.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-0.23-Commit #81 (See https://builds.apache.org/job/Hadoop-Mapreduce-0.23-Commit/81/)
          HADOOP-7753. Support fadvise and sync_file_range in NativeIO. Add ReadaheadPool infrastructure for use in HDFS and MR. Contributed by Todd Lipcon.

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

          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/ReadaheadPool.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/configure.ac
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/file_descriptor.c
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/nativeio/TestNativeIO.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-0.23-Commit #81 (See https://builds.apache.org/job/Hadoop-Mapreduce-0.23-Commit/81/ ) HADOOP-7753 . Support fadvise and sync_file_range in NativeIO. Add ReadaheadPool infrastructure for use in HDFS and MR. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190066 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/ReadaheadPool.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/configure.ac /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/file_descriptor.c /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/nativeio/TestNativeIO.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-0.23-Commit #86 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Commit/86/)
          HADOOP-7753. Support fadvise and sync_file_range in NativeIO. Add ReadaheadPool infrastructure for use in HDFS and MR. Contributed by Todd Lipcon.

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

          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/ReadaheadPool.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/configure.ac
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/file_descriptor.c
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/nativeio/TestNativeIO.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-0.23-Commit #86 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Commit/86/ ) HADOOP-7753 . Support fadvise and sync_file_range in NativeIO. Add ReadaheadPool infrastructure for use in HDFS and MR. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190066 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/ReadaheadPool.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/configure.ac /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/file_descriptor.c /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/nativeio/TestNativeIO.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-0.23-Commit #86 (See https://builds.apache.org/job/Hadoop-Common-0.23-Commit/86/)
          HADOOP-7753. Support fadvise and sync_file_range in NativeIO. Add ReadaheadPool infrastructure for use in HDFS and MR. Contributed by Todd Lipcon.

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

          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/ReadaheadPool.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/configure.ac
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/file_descriptor.c
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/nativeio/TestNativeIO.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-0.23-Commit #86 (See https://builds.apache.org/job/Hadoop-Common-0.23-Commit/86/ ) HADOOP-7753 . Support fadvise and sync_file_range in NativeIO. Add ReadaheadPool infrastructure for use in HDFS and MR. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190066 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/ReadaheadPool.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/configure.ac /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/file_descriptor.c /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/nativeio/TestNativeIO.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #1178 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1178/)
          HADOOP-7753. Support fadvise and sync_file_range in NativeIO. Add ReadaheadPool infrastructure for use in HDFS and MR. Contributed by Todd Lipcon.

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

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/ReadaheadPool.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/configure.ac
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/file_descriptor.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/nativeio/TestNativeIO.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #1178 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1178/ ) HADOOP-7753 . Support fadvise and sync_file_range in NativeIO. Add ReadaheadPool infrastructure for use in HDFS and MR. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190067 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/ReadaheadPool.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/configure.ac /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/file_descriptor.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/nativeio/TestNativeIO.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #1255 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/1255/)
          HADOOP-7753. Support fadvise and sync_file_range in NativeIO. Add ReadaheadPool infrastructure for use in HDFS and MR. Contributed by Todd Lipcon.

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

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/ReadaheadPool.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/configure.ac
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/file_descriptor.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/nativeio/TestNativeIO.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #1255 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/1255/ ) HADOOP-7753 . Support fadvise and sync_file_range in NativeIO. Add ReadaheadPool infrastructure for use in HDFS and MR. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1190067 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/ReadaheadPool.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/configure.ac /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/file_descriptor.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/nativeio/TestNativeIO.java
          Hide
          Todd Lipcon added a comment -

          Committed to 23 and trunk. Thanks for the reviews, Kihwal and Eli.

          Show
          Todd Lipcon added a comment - Committed to 23 and trunk. Thanks for the reviews, Kihwal and Eli.
          Hide
          Eli Collins added a comment -

          +1 nice work!

          Show
          Eli Collins added a comment - +1 nice work!
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12501162/hadoop-7753.txt
          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 tests.

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

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

          -1 findbugs. The patch appears to introduce 1 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:
          org.apache.hadoop.io.retry.TestFailoverProxy

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/334//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/334//artifact/trunk/hadoop-common-project/patchprocess/newPatchFindbugsWarningshadoop-common.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/334//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/12501162/hadoop-7753.txt 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 tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 1 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: org.apache.hadoop.io.retry.TestFailoverProxy +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/334//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/334//artifact/trunk/hadoop-common-project/patchprocess/newPatchFindbugsWarningshadoop-common.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/334//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          Updated patch adds AC_GNU_SOURCE. I wasn't able to reproduce the build issue on my lucid boxes (32-bit or 64-bit)... but hopefully this will fix it.

          Also incorporated Eli's feedback.

          Show
          Todd Lipcon added a comment - Updated patch adds AC_GNU_SOURCE. I wasn't able to reproduce the build issue on my lucid boxes (32-bit or 64-bit)... but hopefully this will fix it. Also incorporated Eli's feedback.
          Hide
          Kihwal Lee added a comment -

          It is a custom macro so autoconf didn't automatically took care of everything. I think HAVE_SYNC_FILE_RANGE has to be checked and _GNU_SOURCE needs to be set before including fcntl.h.

          Show
          Kihwal Lee added a comment - It is a custom macro so autoconf didn't automatically took care of everything. I think HAVE_SYNC_FILE_RANGE has to be checked and _GNU_SOURCE needs to be set before including fcntl.h.
          Hide
          Todd Lipcon added a comment -

          hrm, interesting. Because that implies that HAVE_SYNC_FILE_RANGE got set in config.h... but then you'd think that any other necessary flags would be in CFLAGS or config.h. Hopefully someone will either get me access to the build machines or report back what OS they're running.

          Show
          Todd Lipcon added a comment - hrm, interesting. Because that implies that HAVE_SYNC_FILE_RANGE got set in config.h... but then you'd think that any other necessary flags would be in CFLAGS or config.h. Hopefully someone will either get me access to the build machines or report back what OS they're running.
          Hide
          Kihwal Lee added a comment -

          From the pre-commit build log,
          [INFO] src/org/apache/hadoop/io/nativeio/NativeIO.c:299: warning: implicit declaration of function 'sync_file_range'

          Never a good sign. Is _GNU_SOURCE defined?

          Show
          Kihwal Lee added a comment - From the pre-commit build log, [INFO] src/org/apache/hadoop/io/nativeio/NativeIO.c:299: warning: implicit declaration of function 'sync_file_range' Never a good sign. Is _GNU_SOURCE defined?
          Hide
          Todd Lipcon added a comment -

          Interesting, sync_file_range is getting EINVAL on the build machines. I expected it would give ENOSYS if it had the wrong syscall. I will try to hunt down a similar build machine and figure out what's going on. If I can't figure it out, I'll split this into two patches - one for fadvise and another for sync_file_range, which is less obviously useful.

          Show
          Todd Lipcon added a comment - Interesting, sync_file_range is getting EINVAL on the build machines. I expected it would give ENOSYS if it had the wrong syscall. I will try to hunt down a similar build machine and figure out what's going on. If I can't figure it out, I'll split this into two patches - one for fadvise and another for sync_file_range, which is less obviously useful.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12500988/hadoop-7753.txt
          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 tests.

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

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

          -1 findbugs. The patch appears to introduce 1 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:
          org.apache.hadoop.io.nativeio.TestNativeIO

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/328//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/328//artifact/trunk/hadoop-common-project/patchprocess/newPatchFindbugsWarningshadoop-common.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/328//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/12500988/hadoop-7753.txt 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 tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 1 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: org.apache.hadoop.io.nativeio.TestNativeIO +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/328//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/328//artifact/trunk/hadoop-common-project/patchprocess/newPatchFindbugsWarningshadoop-common.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/328//console This message is automatically generated.
          Hide
          Eli Collins added a comment -

          No new feedback looking at the latest version of the patch.

          Show
          Eli Collins added a comment - No new feedback looking at the latest version of the patch.
          Hide
          Eli Collins added a comment -

          Forgot to add, a small comment in cancel indicating that it doesn't remove the req from the work queue would be good, your rationale makes sense, but removing the req is probably what a user would suspect to be the default behavior.

          Show
          Eli Collins added a comment - Forgot to add, a small comment in cancel indicating that it doesn't remove the req from the work queue would be good, your rationale makes sense, but removing the req is probably what a user would suspect to be the default behavior.
          Hide
          Eli Collins added a comment -

          Aside from Nathan's great feedback I have the following minor comments based on the previous version of the patch. Will check out the latest.

          Shouldn't the catch of the ULE in syncFileRangeIfPossible set syncFileRangePossible to false?

          In manual_sync_file_range in NativeIO.c might be worth referencing RedHat BZ 518581 which has the rationale as to why the glibc API isnt't exposed w the recommendation for the direct syscall.

          In NativeIO.java would be good to javadoc that these methods throw IOEs with the relevant errno.

          I found the wording "each readahead overlaps 50% with the previous readahead" a little confusing (had to read the code to understand the comment), perhaps something like "only issue a new readahead if we've already advanced past the halfway mark of the last one" would be more clear.

          Show
          Eli Collins added a comment - Aside from Nathan's great feedback I have the following minor comments based on the previous version of the patch. Will check out the latest. Shouldn't the catch of the ULE in syncFileRangeIfPossible set syncFileRangePossible to false? In manual_sync_file_range in NativeIO.c might be worth referencing RedHat BZ 518581 which has the rationale as to why the glibc API isnt't exposed w the recommendation for the direct syscall. In NativeIO.java would be good to javadoc that these methods throw IOEs with the relevant errno. I found the wording "each readahead overlaps 50% with the previous readahead" a little confusing (had to read the code to understand the comment), perhaps something like "only issue a new readahead if we've already advanced past the halfway mark of the last one" would be more clear.
          Hide
          Todd Lipcon added a comment -

          slight improvement to test cases so that, if you run tests on a platform without these calls, it will just skip the test instead of failing the test. I ran tests and they passed. Thanks for the reviews, Nathan. I think Eli was also taking a look at this, so I'll wait on his cue to commit.

          Show
          Todd Lipcon added a comment - slight improvement to test cases so that, if you run tests on a platform without these calls, it will just skip the test instead of failing the test. I ran tests and they passed. Thanks for the reviews, Nathan. I think Eli was also taking a look at this, so I'll wait on his cue to commit.
          Hide
          Nathan Roberts added a comment -

          Thanks Todd for the answers. I'm fine with not actively removing cancel'ed requests. I kind of figured the system would have to be in some pretty dire straits for this to ever matter anyway.

          +1 for the patch.

          Show
          Nathan Roberts added a comment - Thanks Todd for the answers. I'm fine with not actively removing cancel'ed requests. I kind of figured the system would have to be in some pretty dire straits for this to ever matter anyway. +1 for the patch.
          Hide
          Todd Lipcon added a comment -

          New version of the patch addresses comments above.

          Show
          Todd Lipcon added a comment - New version of the patch addresses comments above.
          Hide
          Todd Lipcon added a comment -

          Maybe we could expose POOL_SIZE/MAX_POOL_SIZE and do it that way?

          Oops, sorry, I missed this. The idea is that we'll add configurations at the call sites, rather than in ReadaheadPool globally. For example, the patch to integrate this into HDFS adds configs for where (and how much) the DN should do readahead

          Show
          Todd Lipcon added a comment - Maybe we could expose POOL_SIZE/MAX_POOL_SIZE and do it that way? Oops, sorry, I missed this. The idea is that we'll add configurations at the call sites, rather than in ReadaheadPool globally. For example, the patch to integrate this into HDFS adds configs for where (and how much) the DN should do readahead
          Hide
          Todd Lipcon added a comment -

          configure.ac, why the change to ldflags?

          The issue is that the AC_CHECK_LIB macro ends up adding -ljvm to $LIBS when successful. Then all of the following AC_CHECK_FUNCS tests were failing like so:

          configure:11586: gcc -o conftest -g -O2 -I/usr/local/include -L/usr/local/lib conftest.c -ljvm -ldl  >&5
          /usr/bin/ld: cannot find -ljvm
          collect2: ld returned 1 exit status
          configure:11586: $? = 1
          

          so it was failing to properly set HAVE_POSIX_FADVISE.

          Did you happen to test sync_file_range on both 32 and 64 bit architectures with files greater than 4G

          Nope. I unfortunately did not have the chance to test this. But I think if we commit this as an "experimental" feature to start off, then we can address some issues like this down the road. If your team has time to test it, that would be excellent.

          I was wondering if cancel should go ahead and remove the req from the workq.

          It seemed like extra complexity for little gain. In watching this code run with terasorts, etc, the queue was almost always empty (ie most of the time at least one of the RA threads was inactive). If it's OK, I'd prefer to leave as is.

          Regarding the cancel race. It's not guaranteed to return EBADF because the fd is likely to get reused immediately for something else

          True. Let me add a comment as you described.

          It would be nice if ReadaheadRequest sanity checked curPos and maxOffsetToRead as well

          Might be nice in the maxOffsetToRead param to indicate what the "don't care" value is

          Good ideas. Will add.

          Show
          Todd Lipcon added a comment - configure.ac, why the change to ldflags? The issue is that the AC_CHECK_LIB macro ends up adding -ljvm to $LIBS when successful. Then all of the following AC_CHECK_FUNCS tests were failing like so: configure:11586: gcc -o conftest -g -O2 -I/usr/local/include -L/usr/local/lib conftest.c -ljvm -ldl >&5 /usr/bin/ld: cannot find -ljvm collect2: ld returned 1 exit status configure:11586: $? = 1 so it was failing to properly set HAVE_POSIX_FADVISE . Did you happen to test sync_file_range on both 32 and 64 bit architectures with files greater than 4G Nope. I unfortunately did not have the chance to test this. But I think if we commit this as an "experimental" feature to start off, then we can address some issues like this down the road. If your team has time to test it, that would be excellent. I was wondering if cancel should go ahead and remove the req from the workq. It seemed like extra complexity for little gain. In watching this code run with terasorts, etc, the queue was almost always empty (ie most of the time at least one of the RA threads was inactive). If it's OK, I'd prefer to leave as is. Regarding the cancel race. It's not guaranteed to return EBADF because the fd is likely to get reused immediately for something else True. Let me add a comment as you described. It would be nice if ReadaheadRequest sanity checked curPos and maxOffsetToRead as well Might be nice in the maxOffsetToRead param to indicate what the "don't care" value is Good ideas. Will add.
          Hide
          Nathan Roberts added a comment -

          Hi Todd. I reviewed the patch and only have a few questions/suggestions (Nothing that should hold it up):

          • configure.ac, why the change to ldflags?
          • Did you happen to test sync_file_range on both 32 and 64 bit architectures with files greater than 4G? I'm pretty sure it's correct as it basically does exactly what glibc does but I wasn't 100% certain.
          • I was wondering if cancel should go ahead and remove the req from the workq.
          • I think it would nice to have a way to disable readahead via configuration. Maybe we could expose POOL_SIZE/MAX_POOL_SIZE and do it that way?
          • Regarding the cancel race. It's not guaranteed to return EBADF because the fd is likely to get reused immediately for something else. Could be another file, a socket, pretty much anything. I tried to think of ways this could cause a problem, but couldn't come up with any. So I think the code is safe, maybe just a tweak to the comment saying you realize it could be a totally different file and that's ok.
          • It would be nice if ReadaheadRequest sanity checked curPos and maxOffsetToRead as well, that way it's a little easier to guarantee the rest of the method is behaving correctly under all conditions.
          • Might be nice in the maxOffsetToRead param to indicate what the "don't care" value is (Long.MAX_VALUE?)
          Show
          Nathan Roberts added a comment - Hi Todd. I reviewed the patch and only have a few questions/suggestions (Nothing that should hold it up): configure.ac, why the change to ldflags? Did you happen to test sync_file_range on both 32 and 64 bit architectures with files greater than 4G? I'm pretty sure it's correct as it basically does exactly what glibc does but I wasn't 100% certain. I was wondering if cancel should go ahead and remove the req from the workq. I think it would nice to have a way to disable readahead via configuration. Maybe we could expose POOL_SIZE/MAX_POOL_SIZE and do it that way? Regarding the cancel race. It's not guaranteed to return EBADF because the fd is likely to get reused immediately for something else. Could be another file, a socket, pretty much anything. I tried to think of ways this could cause a problem, but couldn't come up with any. So I think the code is safe, maybe just a tweak to the comment saying you realize it could be a totally different file and that's ok. It would be nice if ReadaheadRequest sanity checked curPos and maxOffsetToRead as well, that way it's a little easier to guarantee the rest of the method is behaving correctly under all conditions. Might be nice in the maxOffsetToRead param to indicate what the "don't care" value is (Long.MAX_VALUE?)
          Hide
          Todd Lipcon added a comment -

          The test results and findbugs warning are unrelated. Please review. I have been testing this code on a cluster all week while doing my MR2 testing, it seems quite stable.

          Show
          Todd Lipcon added a comment - The test results and findbugs warning are unrelated. Please review. I have been testing this code on a cluster all week while doing my MR2 testing, it seems quite stable.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12500523/hadoop-7753.txt
          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 tests.

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

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

          -1 findbugs. The patch appears to introduce 1 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 the unit tests build

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/310//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/310//artifact/trunk/hadoop-common-project/patchprocess/newPatchFindbugsWarningshadoop-common.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/310//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/12500523/hadoop-7753.txt 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 tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 1 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 the unit tests build +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/310//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/310//artifact/trunk/hadoop-common-project/patchprocess/newPatchFindbugsWarningshadoop-common.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/310//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          rebased on trunk, and added catch clause for UnsatisfiedLinkError – this allows the new java jar to be used against an older libhadoop.so without crashing (it just disables the new feature)

          Show
          Todd Lipcon added a comment - rebased on trunk, and added catch clause for UnsatisfiedLinkError – this allows the new java jar to be used against an older libhadoop.so without crashing (it just disables the new feature)
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12499488/hadoop-7753.txt
          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 tests.

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

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

          -1 findbugs. The patch appears to introduce 1 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-HADOOP-Build/302//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/302//artifact/trunk/hadoop-common-project/patchprocess/newPatchFindbugsWarningshadoop-common.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/302//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/12499488/hadoop-7753.txt 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 tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 1 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-HADOOP-Build/302//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/302//artifact/trunk/hadoop-common-project/patchprocess/newPatchFindbugsWarningshadoop-common.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/302//console This message is automatically generated.

            People

            • Assignee:
              Todd Lipcon
              Reporter:
              Todd Lipcon
            • Votes:
              0 Vote for this issue
              Watchers:
              16 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development