Uploaded image for project: 'Hadoop HDFS'
  1. Hadoop HDFS
  2. HDFS-8562

HDFS Performance is impacted by FileInputStream Finalizer

    Details

    • Type: Improvement
    • Status: Patch Available
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: 2.5.0
    • Fix Version/s: None
    • Component/s: datanode, performance
    • Labels:
      None
    • Environment:

      Impact any application that uses HDFS

      Description

      While running HBase using HDFS as datanodes, we noticed excessive high GC pause spikes. For example with jdk8 update 40 and G1 collector, we saw datanode GC pauses spiked toward 160 milliseconds while they should be around 20 milliseconds.
      We tracked down to GC logs and found those long GC pauses were devoted to process high number of final references.

      For example, this Young GC:
      2715.501: [GC pause (G1 Evacuation Pause) (young) 0.1529017 secs]
      2715.572: [SoftReference, 0 refs, 0.0001034 secs]
      2715.572: [WeakReference, 0 refs, 0.0000123 secs]
      2715.572: [FinalReference, 8292 refs, 0.0748194 secs]
      2715.647: [PhantomReference, 0 refs, 160 refs, 0.0001333 secs]
      2715.647: [JNI Weak Reference, 0.0000140 secs]
      [Ref Proc: 122.3 ms]
      [Eden: 910.0M(910.0M)->0.0B(911.0M) Survivors: 11.0M->10.0M Heap: 951.1M(1536.0M)->40.2M(1536.0M)]
      [Times: user=0.47 sys=0.01, real=0.15 secs]

      This young GC took 152.9 milliseconds STW pause, while spent 122.3 milliseconds in Ref Proc, which processed 8292 FinalReference in 74.8 milliseconds plus some overhead.

      We used JFR and JMAP with Memory Analyzer to track down and found those FinalReference were all from FileInputStream. We checked HDFS code and saw the use of the FileInputStream in datanode:
      https://apache.googlesource.com/hadoop-common/+/refs/heads/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/MappableBlock.java

      1.	public static MappableBlock load(long length,
      2.	FileInputStream blockIn, FileInputStream metaIn,
      3.	String blockFileName) throws IOException {
      4.	MappableBlock mappableBlock = null;
      5.	MappedByteBuffer mmap = null;
      6.	FileChannel blockChannel = null;
      7.	try {
      8.	blockChannel = blockIn.getChannel();
      9.	if (blockChannel == null) {
      10.	throw new IOException("Block InputStream has no FileChannel.");
      11.	}
      12.	mmap = blockChannel.map(MapMode.READ_ONLY, 0, length);
      13.	NativeIO.POSIX.getCacheManipulator().mlock(blockFileName, mmap, length);
      14.	verifyChecksum(length, metaIn, blockChannel, blockFileName);
      15.	mappableBlock = new MappableBlock(mmap, length);
      16.	} finally {
      17.	IOUtils.closeQuietly(blockChannel);
      18.	if (mappableBlock == null) {
      19.	if (mmap != null) {
      20.	NativeIO.POSIX.munmap(mmap); // unmapping also unlocks
      21.	}
      22.	}
      23.	}
      24.	return mappableBlock;
      25.	}
      

      We looked up https://docs.oracle.com/javase/7/docs/api/java/io/FileInputStream.html and
      http://hg.openjdk.java.net/jdk7/jdk7/jdk/file/23bdcede4e39/src/share/classes/java/io/FileInputStream.java and noticed FileInputStream relies on the Finalizer to release its resource.

      When a class that has a finalizer created, an entry for that class instance is put on a queue in the JVM so the JVM knows it has a finalizer that needs to be executed.

      The current issue is: even with programmers do call close() after using FileInputStream, its finalize() method will still be called. In other words, still get the side effect of the FinalReference being registered at FileInputStream allocation time, and also reference processing to reclaim the FinalReference during GC (any GC solution has to deal with this).

      We can imagine When running industry deployment HDFS, millions of files could be opened and closed which resulted in a very large number of finalizers being registered and subsequently being executed. That could cause very long GC pause times.

      We tried to use Files.newInputStream() to replace FileInputStream, but it was clear we could not replace FileInputStream in hdfs/server/datanode/fsdataset/impl/MappableBlock.java

      We notified Oracle JVM team of this performance issue that impacting all Big Data applications using HDFS. We recommended the proper fix in Java SE FileInputStream. Because (1) it is really nothing wrong to use FileInputStream in above datanode code, (2) as the object with a finalizer is registered with finalizer list within the JVM at object allocation time, if someone makes an explicit call to close or free the resources that are to be done in the finalizer, then the finalizer should be pulled off the internal JVM’s finalizer list. That will release the JVM from having to treat the object as special because it has a finalizer, i.e. no need for GC to execute the finalizer as part of Reference Processing.

      As the java fix involves both JVM code and Java SE code, it might take time for the full solution to be available in future JDK releases. We would like to file his JIRA to notify Big Data, HDFS community to aware this issue while using HDFS and while writing code using FileInputStream

      One alternative is to use Files.newInputStream() to substitute FileInputStream if it is possible. File.newInputStream() will give an InputStream and do so in a manner that does not include a finalizer.

      We welcome HDFS community to discuss this issue and see if there are additional ideas to solve this problem.

      1. HDFS-8562.01.patch
        38 kB
        Wei Zhou
      2. HDFS-8562.002b.patch
        41 kB
        Colin P. McCabe
      3. HDFS-8562.003a.patch
        50 kB
        Kai Zheng
      4. HDFS-8562.003b.patch
        59 kB
        Kai Zheng
      5. HDFS-8562.004a.patch
        64 kB
        Kai Zheng
      6. HDFS-8562.004b.patch
        60 kB
        Kai Zheng

        Activity

        Hide
        cmccabe Colin P. McCabe added a comment -

        While it's true that we use FileInputStream / FileOutputStream in many places, most of those places have short-lived objects or only use very small numbers of objects. Like I mentioned earlier, the big problem with finalizers we encounter is in the short-circuit read stream cache. If we can fix that, as this patch attempts to do, we will have eliminated most of the problem. I believe we can come up with a patch that does this for both jdk6 and jdk7, and has a fallback for other jdks.

        Thanks for filing the Oracle JIRAs. They are a step in the right direction. Of course, it will be many years before we can drop support for the current jdks, so we also need the patch on this jira, I believe.

        Show
        cmccabe Colin P. McCabe added a comment - While it's true that we use FileInputStream / FileOutputStream in many places, most of those places have short-lived objects or only use very small numbers of objects. Like I mentioned earlier, the big problem with finalizers we encounter is in the short-circuit read stream cache. If we can fix that, as this patch attempts to do, we will have eliminated most of the problem. I believe we can come up with a patch that does this for both jdk6 and jdk7, and has a fallback for other jdks. Thanks for filing the Oracle JIRAs. They are a step in the right direction. Of course, it will be many years before we can drop support for the current jdks, so we also need the patch on this jira, I believe.
        Hide
        drankye Kai Zheng added a comment -

        Hi Yanping,

        Of course, it is just in theory, not practical, as it will likely re-design entire HDFS.

        No, it won't be like that. Either way would only bring limited impact to HDFS in restricted scopes.

        there is really no need to fix FileInputStream and FileOutputStream as new code can be written using above new functions, right?

        I thought Colin had a comment above that tells even we have the perfect API and solution for the issue you reported here, we'll still need a fix like the patch attached for previous JDK versions.

        Show
        drankye Kai Zheng added a comment - Hi Yanping, Of course, it is just in theory, not practical, as it will likely re-design entire HDFS. No, it won't be like that. Either way would only bring limited impact to HDFS in restricted scopes. there is really no need to fix FileInputStream and FileOutputStream as new code can be written using above new functions, right? I thought Colin had a comment above that tells even we have the perfect API and solution for the issue you reported here, we'll still need a fix like the patch attached for previous JDK versions.
        Hide
        ywang261 Yanping Wang added a comment -

        Hi, Kai and Colin

        When Doug Cutting developed HDFS in 2005. There was JDK1.5, which only had FIS/FOS. HDFS was developed heavily relying on FIS/FOS to share and use opened FDs.

        In 2011, JDK1.7, https://docs.oracle.com/javase/7/docs/api/java/nio/channels/Channels.html Oracle provided Channels.newInputStream and Channels.newOutputStream. If Oracle also provide Public static FileChannel open(FileDescriptor fd, boolean readable, boolean writable, boolean append), in theory, all FileInputStream and FileOutputStream code can be replaced using Channels.newInputStream and Channels.newOutputStream, and public static FileChannel open(FileDescriptor fd, boolean readable, boolean writable, boolean append), right? Of course, it is just in theory, not practical, as it will likely re-design entire HDFS.
        I want to make sure, if Oracle provides Public static FileChannel open(FileDescriptor fd, boolean readable, boolean writable, boolean append), there is really no need to fix FileInputStream and FileOutputStream as new code can be written using above new functions, right?

        Thanks

        Show
        ywang261 Yanping Wang added a comment - Hi, Kai and Colin When Doug Cutting developed HDFS in 2005. There was JDK1.5, which only had FIS/FOS. HDFS was developed heavily relying on FIS/FOS to share and use opened FDs. In 2011, JDK1.7, https://docs.oracle.com/javase/7/docs/api/java/nio/channels/Channels.html Oracle provided Channels.newInputStream and Channels.newOutputStream. If Oracle also provide Public static FileChannel open(FileDescriptor fd, boolean readable, boolean writable, boolean append), in theory, all FileInputStream and FileOutputStream code can be replaced using Channels.newInputStream and Channels.newOutputStream, and public static FileChannel open(FileDescriptor fd, boolean readable, boolean writable, boolean append), right? Of course, it is just in theory, not practical, as it will likely re-design entire HDFS. I want to make sure, if Oracle provides Public static FileChannel open(FileDescriptor fd, boolean readable, boolean writable, boolean append), there is really no need to fix FileInputStream and FileOutputStream as new code can be written using above new functions, right? Thanks
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 0s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.
        +1 mvninstall 7m 58s trunk passed
        +1 compile 8m 41s trunk passed with JDK v1.8.0_66
        +1 compile 9m 34s trunk passed with JDK v1.7.0_91
        +1 checkstyle 1m 5s trunk passed
        +1 mvnsite 2m 40s trunk passed
        +1 mvneclipse 0m 42s trunk passed
        +1 findbugs 6m 2s trunk passed
        +1 javadoc 2m 28s trunk passed with JDK v1.8.0_66
        +1 javadoc 3m 25s trunk passed with JDK v1.7.0_91
        +1 mvninstall 3m 2s the patch passed
        +1 compile 8m 43s the patch passed with JDK v1.8.0_66
        -1 javac 19m 57s root-jdk1.8.0_66 with JDK v1.8.0_66 generated 2 new issues (was 731, now 733).
        +1 javac 8m 43s the patch passed
        +1 compile 9m 24s the patch passed with JDK v1.7.0_91
        -1 javac 29m 21s root-jdk1.7.0_91 with JDK v1.7.0_91 generated 2 new issues (was 724, now 726).
        +1 javac 9m 24s the patch passed
        -1 checkstyle 1m 7s Patch generated 4 new checkstyle issues in root (total was 240, now 237).
        +1 mvnsite 2m 40s the patch passed
        +1 mvneclipse 0m 41s the patch passed
        +1 whitespace 0m 0s Patch has no whitespace issues.
        +1 findbugs 6m 32s the patch passed
        +1 javadoc 2m 32s the patch passed with JDK v1.8.0_66
        +1 javadoc 3m 27s the patch passed with JDK v1.7.0_91
        -1 unit 7m 55s hadoop-common in the patch failed with JDK v1.8.0_66.
        +1 unit 0m 56s hadoop-hdfs-client in the patch passed with JDK v1.8.0_66.
        -1 unit 57m 2s hadoop-hdfs in the patch failed with JDK v1.8.0_66.
        +1 unit 8m 54s hadoop-common in the patch passed with JDK v1.7.0_91.
        +1 unit 1m 3s hadoop-hdfs-client in the patch passed with JDK v1.7.0_91.
        -1 unit 59m 1s hadoop-hdfs in the patch failed with JDK v1.7.0_91.
        +1 asflicense 0m 27s Patch does not generate ASF License warnings.
        217m 53s



        Reason Tests
        JDK v1.8.0_66 Failed junit tests hadoop.fs.TestLocalFsFCStatistics
          hadoop.hdfs.server.datanode.TestBlockScanner
          hadoop.hdfs.TestLeaseRecovery2
          hadoop.hdfs.server.namenode.TestNNThroughputBenchmark
        JDK v1.7.0_91 Failed junit tests hadoop.hdfs.TestAclsEndToEnd
          hadoop.hdfs.server.datanode.TestBlockScanner
          hadoop.hdfs.qjournal.TestSecureNNWithQJM



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:0ca8df7
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12781135/HDFS-8562.004b.patch
        JIRA Issue HDFS-8562
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux bd23b7b58c4e 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 89022f8
        Default Java 1.7.0_91
        Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_66 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_91
        findbugs v3.0.0
        javac root-jdk1.8.0_66: https://builds.apache.org/job/PreCommit-HDFS-Build/14065/artifact/patchprocess/diff-compile-javac-root-jdk1.8.0_66.txt
        javac root-jdk1.7.0_91: https://builds.apache.org/job/PreCommit-HDFS-Build/14065/artifact/patchprocess/diff-compile-javac-root-jdk1.7.0_91.txt
        checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/14065/artifact/patchprocess/diff-checkstyle-root.txt
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/14065/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_66.txt
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/14065/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_66.txt
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/14065/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_91.txt
        unit test logs https://builds.apache.org/job/PreCommit-HDFS-Build/14065/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_66.txt https://builds.apache.org/job/PreCommit-HDFS-Build/14065/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_66.txt https://builds.apache.org/job/PreCommit-HDFS-Build/14065/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_91.txt
        JDK v1.7.0_91 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/14065/testReport/
        modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs-client U: .
        Max memory used 76MB
        Powered by Apache Yetus 0.2.0-SNAPSHOT http://yetus.apache.org
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/14065/console

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 5 new or modified test files. +1 mvninstall 7m 58s trunk passed +1 compile 8m 41s trunk passed with JDK v1.8.0_66 +1 compile 9m 34s trunk passed with JDK v1.7.0_91 +1 checkstyle 1m 5s trunk passed +1 mvnsite 2m 40s trunk passed +1 mvneclipse 0m 42s trunk passed +1 findbugs 6m 2s trunk passed +1 javadoc 2m 28s trunk passed with JDK v1.8.0_66 +1 javadoc 3m 25s trunk passed with JDK v1.7.0_91 +1 mvninstall 3m 2s the patch passed +1 compile 8m 43s the patch passed with JDK v1.8.0_66 -1 javac 19m 57s root-jdk1.8.0_66 with JDK v1.8.0_66 generated 2 new issues (was 731, now 733). +1 javac 8m 43s the patch passed +1 compile 9m 24s the patch passed with JDK v1.7.0_91 -1 javac 29m 21s root-jdk1.7.0_91 with JDK v1.7.0_91 generated 2 new issues (was 724, now 726). +1 javac 9m 24s the patch passed -1 checkstyle 1m 7s Patch generated 4 new checkstyle issues in root (total was 240, now 237). +1 mvnsite 2m 40s the patch passed +1 mvneclipse 0m 41s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. +1 findbugs 6m 32s the patch passed +1 javadoc 2m 32s the patch passed with JDK v1.8.0_66 +1 javadoc 3m 27s the patch passed with JDK v1.7.0_91 -1 unit 7m 55s hadoop-common in the patch failed with JDK v1.8.0_66. +1 unit 0m 56s hadoop-hdfs-client in the patch passed with JDK v1.8.0_66. -1 unit 57m 2s hadoop-hdfs in the patch failed with JDK v1.8.0_66. +1 unit 8m 54s hadoop-common in the patch passed with JDK v1.7.0_91. +1 unit 1m 3s hadoop-hdfs-client in the patch passed with JDK v1.7.0_91. -1 unit 59m 1s hadoop-hdfs in the patch failed with JDK v1.7.0_91. +1 asflicense 0m 27s Patch does not generate ASF License warnings. 217m 53s Reason Tests JDK v1.8.0_66 Failed junit tests hadoop.fs.TestLocalFsFCStatistics   hadoop.hdfs.server.datanode.TestBlockScanner   hadoop.hdfs.TestLeaseRecovery2   hadoop.hdfs.server.namenode.TestNNThroughputBenchmark JDK v1.7.0_91 Failed junit tests hadoop.hdfs.TestAclsEndToEnd   hadoop.hdfs.server.datanode.TestBlockScanner   hadoop.hdfs.qjournal.TestSecureNNWithQJM Subsystem Report/Notes Docker Image:yetus/hadoop:0ca8df7 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12781135/HDFS-8562.004b.patch JIRA Issue HDFS-8562 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux bd23b7b58c4e 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 89022f8 Default Java 1.7.0_91 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_66 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_91 findbugs v3.0.0 javac root-jdk1.8.0_66: https://builds.apache.org/job/PreCommit-HDFS-Build/14065/artifact/patchprocess/diff-compile-javac-root-jdk1.8.0_66.txt javac root-jdk1.7.0_91: https://builds.apache.org/job/PreCommit-HDFS-Build/14065/artifact/patchprocess/diff-compile-javac-root-jdk1.7.0_91.txt checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/14065/artifact/patchprocess/diff-checkstyle-root.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/14065/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_66.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/14065/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_66.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/14065/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_91.txt unit test logs https://builds.apache.org/job/PreCommit-HDFS-Build/14065/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_66.txt https://builds.apache.org/job/PreCommit-HDFS-Build/14065/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_66.txt https://builds.apache.org/job/PreCommit-HDFS-Build/14065/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_91.txt JDK v1.7.0_91 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/14065/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs-client U: . Max memory used 76MB Powered by Apache Yetus 0.2.0-SNAPSHOT http://yetus.apache.org Console output https://builds.apache.org/job/PreCommit-HDFS-Build/14065/console This message was automatically generated.
        Hide
        drankye Kai Zheng added a comment -

        Rebased the patch.

        Show
        drankye Kai Zheng added a comment - Rebased the patch.
        Hide
        drankye Kai Zheng added a comment -

        Hi Yanping,

        That sounds great and thanks for your effort!

        they agree it is reasonable to create a FileChannel from a FileDescriptor.

        Yeah, this is the main requirement. How about the other one made by Colin P. McCabe saying "add a getFileDescriptor method to FileChannel"?

        With a FileChennel created by FD, we still need to determine if the underlying FD was opened for append, for read, or write. It is not difficult to do, HDFS needs add a line to find out FD flags using Unix fcntl. will it work for you?

        Hmm, I guess it can work, but it would rely on implementing a native method for that. For convenient, would it be good to have the new API like follows? The callers of the new API would be better to determine and pass the open flags (readable, writable and appendable) to it.

            public static FileChannel open(FileDescriptor fd, boolean readable, boolean writable, boolean append)
        

        Thanks!

        Show
        drankye Kai Zheng added a comment - Hi Yanping, That sounds great and thanks for your effort! they agree it is reasonable to create a FileChannel from a FileDescriptor. Yeah, this is the main requirement. How about the other one made by Colin P. McCabe saying "add a getFileDescriptor method to FileChannel"? With a FileChennel created by FD, we still need to determine if the underlying FD was opened for append, for read, or write. It is not difficult to do, HDFS needs add a line to find out FD flags using Unix fcntl. will it work for you? Hmm, I guess it can work, but it would rely on implementing a native method for that. For convenient, would it be good to have the new API like follows? The callers of the new API would be better to determine and pass the open flags (readable, writable and appendable) to it. public static FileChannel open(FileDescriptor fd, boolean readable, boolean writable, boolean append) Thanks!
        Hide
        ywang261 Yanping Wang added a comment -

        Hi, Kai and Colin

        I got update from Oracle Java team, they agree it is reasonable to create a FileChannel from a FileDescriptor. They are investigating additional use-cases and motivation to justify the efforts. With a FileChennel created by FD, we still need to determine if the underlying FD was opened for append, for read, or write. It is not difficult to do, HDFS needs add a line to find out FD flags using Unix fcntl. will it work for you?
        I am not sure if they can get it done on jdk8 releases, if there is JSR filed to record this request, I will post here.

        Show
        ywang261 Yanping Wang added a comment - Hi, Kai and Colin I got update from Oracle Java team, they agree it is reasonable to create a FileChannel from a FileDescriptor. They are investigating additional use-cases and motivation to justify the efforts. With a FileChennel created by FD, we still need to determine if the underlying FD was opened for append, for read, or write. It is not difficult to do, HDFS needs add a line to find out FD flags using Unix fcntl. will it work for you? I am not sure if they can get it done on jdk8 releases, if there is JSR filed to record this request, I will post here.
        Hide
        drankye Kai Zheng added a comment -

        Hi Colin P. McCabe, I may misunderstood you so to be clear, lets ref. the following codes:

          public static PassedFileChannel open(FileDescriptor fd,
                                               boolean writable) throws IOException {
            FileChannel channel = null;
            try {
              if (open5Args != null) {
                /**
                 * Attempt this private method existing in some JDK:
                 * public static FileChannel open(FileDescriptor fd, String path,
                 *                  boolean readable, boolean writable, Object parent)
                 * Note the path is expected to be nullable.
                 */
                channel = (FileChannel) open5Args.invoke(null,
                    fd, null, true, writable, null);
              } else if (open4Args != null) {
                /**
                 * Attempt this private method existing in some JDK:
                 * public static FileChannel open(FileDescriptor fd, boolean readable,
                 *                                boolean writable, Object parent)
                 */
                channel = (FileChannel) open4Args.invoke(null,
                    fd, true, writable, null);
              }
            } catch (IllegalArgumentException e) {
              // Ignore any error due to unexpected JDKs, or JDK versions.
            } catch (InvocationTargetException e) {
              // Ignore
            } catch (IllegalAccessException e) {
              // Ignore
            }
        
            if (channel != null) {
              return new PassedFileChannel(channel, fd);
            }
        
            // All errors will fallback to this, as before using FileInputStream.
            FileInputStream fis = new FileInputStream(fd);
            return new PassedFileChannel(fis);
          }
        

        The try block catches 3 kinds of exceptions like IllegalArgumentException and when either of them happens, channel will be null. I might not be clear, but what I mean is, the exceptions particularly IllegalArgumentException may happen, because in the open5Args case, path is required but we always pass it null. This may work in some JDK versions, but may be not for others. We bet it be nullable but we may be incorrect as the method is a private one subject to implementation change. Sure if you'd insist that we don't worry about this, I'm OK. You said we do not do any fallback, in somewhere or in the function at all? I thought at least in the coding form we need to catch these exceptions for reflection and when some happens we would need the fallback just in case. Would you please help clarify? Thanks.

        By the way, I'm handling some minor issues found here separately in HADOOP-12658 as I'm not sure this will be in or not.

        Show
        drankye Kai Zheng added a comment - Hi Colin P. McCabe , I may misunderstood you so to be clear, lets ref. the following codes: public static PassedFileChannel open(FileDescriptor fd, boolean writable) throws IOException { FileChannel channel = null ; try { if (open5Args != null ) { /** * Attempt this private method existing in some JDK: * public static FileChannel open(FileDescriptor fd, String path, * boolean readable, boolean writable, Object parent) * Note the path is expected to be nullable. */ channel = (FileChannel) open5Args.invoke( null , fd, null , true , writable, null ); } else if (open4Args != null ) { /** * Attempt this private method existing in some JDK: * public static FileChannel open(FileDescriptor fd, boolean readable, * boolean writable, Object parent) */ channel = (FileChannel) open4Args.invoke( null , fd, true , writable, null ); } } catch (IllegalArgumentException e) { // Ignore any error due to unexpected JDKs, or JDK versions. } catch (InvocationTargetException e) { // Ignore } catch (IllegalAccessException e) { // Ignore } if (channel != null ) { return new PassedFileChannel(channel, fd); } // All errors will fallback to this , as before using FileInputStream. FileInputStream fis = new FileInputStream(fd); return new PassedFileChannel(fis); } The try block catches 3 kinds of exceptions like IllegalArgumentException and when either of them happens, channel will be null . I might not be clear, but what I mean is, the exceptions particularly IllegalArgumentException may happen, because in the open5Args case, path is required but we always pass it null. This may work in some JDK versions, but may be not for others. We bet it be nullable but we may be incorrect as the method is a private one subject to implementation change. Sure if you'd insist that we don't worry about this, I'm OK. You said we do not do any fallback, in somewhere or in the function at all? I thought at least in the coding form we need to catch these exceptions for reflection and when some happens we would need the fallback just in case. Would you please help clarify? Thanks. By the way, I'm handling some minor issues found here separately in HADOOP-12658 as I'm not sure this will be in or not.
        Hide
        drankye Kai Zheng added a comment -

        I thought this may be pending some while for more reviews as it's dealing with the performance concern issue not obviously. The javadoc and check style issues found here would be better to be in sooner on the other hand so I would handle them separately in HDFS-12658. Will rebase this after that.

        Show
        drankye Kai Zheng added a comment - I thought this may be pending some while for more reviews as it's dealing with the performance concern issue not obviously. The javadoc and check style issues found here would be better to be in sooner on the other hand so I would handle them separately in HDFS-12658 . Will rebase this after that.
        Hide
        cmccabe Colin P. McCabe added a comment -

        Thanks, Kai Zheng.

        I'm worrying about a chance that the method may fail due to illegal arguments we passed. It may happen because it's an internal method, for example, right now the path may be null, but then in some time later the path becomes a must. It may be safer if we would capture such exceptions and go to fallbacks.

        Hmm. At the moment, the function is not catching any exceptions-- it's just checking for null. I think it's unlikely that the method will simply return null on the case of error rather than throwing an appropriate exception. Our reflection calls are already checking the argument types of the function, so I'm not sure that it's worth worrying about this.

        Looks like DomainSocket#receiveFileDescriptors isn't used anywhere. Would we rearrange the codes to use it or remove it at all?

        Yeah, we can remove the public method. I don't remember if we ever used it, but we certainly don't now.

        I'm about to go on vacation for 2 weeks, so I apologize if reviews become slower. If someone else wants to review, then I won't feel bad.

        Show
        cmccabe Colin P. McCabe added a comment - Thanks, Kai Zheng . I'm worrying about a chance that the method may fail due to illegal arguments we passed. It may happen because it's an internal method, for example, right now the path may be null, but then in some time later the path becomes a must. It may be safer if we would capture such exceptions and go to fallbacks. Hmm. At the moment, the function is not catching any exceptions-- it's just checking for null. I think it's unlikely that the method will simply return null on the case of error rather than throwing an appropriate exception. Our reflection calls are already checking the argument types of the function, so I'm not sure that it's worth worrying about this. Looks like DomainSocket#receiveFileDescriptors isn't used anywhere. Would we rearrange the codes to use it or remove it at all? Yeah, we can remove the public method. I don't remember if we ever used it, but we certainly don't now. I'm about to go on vacation for 2 weeks, so I apologize if reviews become slower. If someone else wants to review, then I won't feel bad.
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 0s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.
        +1 mvninstall 7m 29s trunk passed
        +1 compile 7m 52s trunk passed with JDK v1.8.0_66
        +1 compile 8m 37s trunk passed with JDK v1.7.0_91
        +1 checkstyle 0m 56s trunk passed
        +1 mvnsite 2m 26s trunk passed
        +1 mvneclipse 0m 41s trunk passed
        +1 findbugs 5m 29s trunk passed
        +1 javadoc 2m 15s trunk passed with JDK v1.8.0_66
        +1 javadoc 3m 12s trunk passed with JDK v1.7.0_91
        +1 mvninstall 2m 59s the patch passed
        +1 compile 8m 10s the patch passed with JDK v1.8.0_66
        -1 javac 18m 32s root-jdk1.8.0_66 with JDK v1.8.0_66 generated 2 new issues (was 730, now 732).
        +1 javac 8m 10s the patch passed
        +1 compile 8m 45s the patch passed with JDK v1.7.0_91
        -1 javac 27m 17s root-jdk1.7.0_91 with JDK v1.7.0_91 generated 2 new issues (was 724, now 726).
        +1 javac 8m 45s the patch passed
        -1 checkstyle 0m 58s Patch generated 1 new checkstyle issues in root (total was 253, now 240).
        +1 mvnsite 2m 30s the patch passed
        +1 mvneclipse 0m 41s the patch passed
        +1 whitespace 0m 0s Patch has no whitespace issues.
        +1 findbugs 5m 52s the patch passed
        +1 javadoc 2m 20s the patch passed with JDK v1.8.0_66
        +1 javadoc 3m 14s the patch passed with JDK v1.7.0_91
        -1 unit 6m 43s hadoop-common in the patch failed with JDK v1.8.0_66.
        +1 unit 0m 51s hadoop-hdfs-client in the patch passed with JDK v1.8.0_66.
        -1 unit 58m 49s hadoop-hdfs in the patch failed with JDK v1.8.0_66.
        -1 unit 12m 58s hadoop-common in the patch failed with JDK v1.7.0_91.
        +1 unit 1m 4s hadoop-hdfs-client in the patch passed with JDK v1.7.0_91.
        -1 unit 51m 53s hadoop-hdfs in the patch failed with JDK v1.7.0_91.
        +1 asflicense 0m 26s Patch does not generate ASF License warnings.
        208m 46s



        Reason Tests
        JDK v1.8.0_66 Failed junit tests hadoop.fs.shell.TestCopyPreserveFlag
          hadoop.metrics2.impl.TestGangliaMetrics
          hadoop.hdfs.server.datanode.TestFsDatasetCache
          hadoop.hdfs.server.namenode.TestNNThroughputBenchmark
          hadoop.hdfs.server.datanode.TestBlockReplacement
          hadoop.hdfs.server.datanode.TestBlockScanner
          hadoop.hdfs.server.blockmanagement.TestReplicationPolicyConsiderLoad
        JDK v1.7.0_91 Failed junit tests hadoop.metrics2.impl.TestGangliaMetrics
          hadoop.hdfs.server.namenode.TestNameEditsConfigs
          hadoop.hdfs.server.blockmanagement.TestReplicationPolicyConsiderLoad



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:0ca8df7
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12779208/HDFS-8562.004a.patch
        JIRA Issue HDFS-8562
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 8b1960ad28fe 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 8c180a1
        findbugs v3.0.0
        javac root-jdk1.8.0_66: https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/diff-compile-javac-root-jdk1.8.0_66.txt
        javac root-jdk1.7.0_91: https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/diff-compile-javac-root-jdk1.7.0_91.txt
        checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/diff-checkstyle-root.txt
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_66.txt
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_66.txt
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_91.txt
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_91.txt
        unit test logs https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_66.txt https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_66.txt https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_91.txt https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_91.txt
        JDK v1.7.0_91 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/13983/testReport/
        modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs-client U: .
        Max memory used 76MB
        Powered by Apache Yetus 0.2.0-SNAPSHOT http://yetus.apache.org
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/13983/console

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 5 new or modified test files. +1 mvninstall 7m 29s trunk passed +1 compile 7m 52s trunk passed with JDK v1.8.0_66 +1 compile 8m 37s trunk passed with JDK v1.7.0_91 +1 checkstyle 0m 56s trunk passed +1 mvnsite 2m 26s trunk passed +1 mvneclipse 0m 41s trunk passed +1 findbugs 5m 29s trunk passed +1 javadoc 2m 15s trunk passed with JDK v1.8.0_66 +1 javadoc 3m 12s trunk passed with JDK v1.7.0_91 +1 mvninstall 2m 59s the patch passed +1 compile 8m 10s the patch passed with JDK v1.8.0_66 -1 javac 18m 32s root-jdk1.8.0_66 with JDK v1.8.0_66 generated 2 new issues (was 730, now 732). +1 javac 8m 10s the patch passed +1 compile 8m 45s the patch passed with JDK v1.7.0_91 -1 javac 27m 17s root-jdk1.7.0_91 with JDK v1.7.0_91 generated 2 new issues (was 724, now 726). +1 javac 8m 45s the patch passed -1 checkstyle 0m 58s Patch generated 1 new checkstyle issues in root (total was 253, now 240). +1 mvnsite 2m 30s the patch passed +1 mvneclipse 0m 41s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. +1 findbugs 5m 52s the patch passed +1 javadoc 2m 20s the patch passed with JDK v1.8.0_66 +1 javadoc 3m 14s the patch passed with JDK v1.7.0_91 -1 unit 6m 43s hadoop-common in the patch failed with JDK v1.8.0_66. +1 unit 0m 51s hadoop-hdfs-client in the patch passed with JDK v1.8.0_66. -1 unit 58m 49s hadoop-hdfs in the patch failed with JDK v1.8.0_66. -1 unit 12m 58s hadoop-common in the patch failed with JDK v1.7.0_91. +1 unit 1m 4s hadoop-hdfs-client in the patch passed with JDK v1.7.0_91. -1 unit 51m 53s hadoop-hdfs in the patch failed with JDK v1.7.0_91. +1 asflicense 0m 26s Patch does not generate ASF License warnings. 208m 46s Reason Tests JDK v1.8.0_66 Failed junit tests hadoop.fs.shell.TestCopyPreserveFlag   hadoop.metrics2.impl.TestGangliaMetrics   hadoop.hdfs.server.datanode.TestFsDatasetCache   hadoop.hdfs.server.namenode.TestNNThroughputBenchmark   hadoop.hdfs.server.datanode.TestBlockReplacement   hadoop.hdfs.server.datanode.TestBlockScanner   hadoop.hdfs.server.blockmanagement.TestReplicationPolicyConsiderLoad JDK v1.7.0_91 Failed junit tests hadoop.metrics2.impl.TestGangliaMetrics   hadoop.hdfs.server.namenode.TestNameEditsConfigs   hadoop.hdfs.server.blockmanagement.TestReplicationPolicyConsiderLoad Subsystem Report/Notes Docker Image:yetus/hadoop:0ca8df7 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12779208/HDFS-8562.004a.patch JIRA Issue HDFS-8562 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 8b1960ad28fe 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 8c180a1 findbugs v3.0.0 javac root-jdk1.8.0_66: https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/diff-compile-javac-root-jdk1.8.0_66.txt javac root-jdk1.7.0_91: https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/diff-compile-javac-root-jdk1.7.0_91.txt checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/diff-checkstyle-root.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_66.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_66.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_91.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_91.txt unit test logs https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_66.txt https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_66.txt https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_91.txt https://builds.apache.org/job/PreCommit-HDFS-Build/13983/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_91.txt JDK v1.7.0_91 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/13983/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs-client U: . Max memory used 76MB Powered by Apache Yetus 0.2.0-SNAPSHOT http://yetus.apache.org Console output https://builds.apache.org/job/PreCommit-HDFS-Build/13983/console This message was automatically generated.
        Hide
        drankye Kai Zheng added a comment -

        Updated the patch according to review comments and discussion above. It also cleared various new and existing checking style and FindBugs issues.

        Show
        drankye Kai Zheng added a comment - Updated the patch according to review comments and discussion above. It also cleared various new and existing checking style and FindBugs issues.
        Hide
        drankye Kai Zheng added a comment -

        we should just call that method and not do any fallbacks.

        I'm worrying about a chance that the method may fail due to illegal arguments we passed. It may happen because it's an internal method, for example, right now the path may be null, but then in some time later the path becomes a must. It may be safer if we would capture such exceptions and go to fallbacks.

        Show
        drankye Kai Zheng added a comment - we should just call that method and not do any fallbacks. I'm worrying about a chance that the method may fail due to illegal arguments we passed. It may happen because it's an internal method, for example, right now the path may be null, but then in some time later the path becomes a must. It may be safer if we would capture such exceptions and go to fallbacks.
        Hide
        drankye Kai Zheng added a comment -

        Thanks Colin for the detailed comments. They sound good to me and I will update.
        Looks like DomainSocket#receiveFileDescriptors isn't used anywhere. Would we rearrange the codes to use it or remove it at all?

        Show
        drankye Kai Zheng added a comment - Thanks Colin for the detailed comments. They sound good to me and I will update. Looks like DomainSocket#receiveFileDescriptors isn't used anywhere. Would we rearrange the codes to use it or remove it at all?
        Hide
        cmccabe Colin P. McCabe added a comment -

        Thanks, Kai Zheng.

        public void createDescriptor(String info, int length,
                                      PassedFileChannel[] fchan)
        

        This can simply return a PassedFileChannel, now that we only need to return a single thing rather than two.

        * @throws IOException       If the accept timed out.
        

        SocketTimeoutException is timeouts, other IOExceptions could be other things. Maybe have two lines about exceptions, one for IOE and another for timeout?

        recvFileChannels: you need to set descriptors[i] = null once you have created a PassedFileChannel for the descriptor. Otherwise you would try to close the same descriptor twice after an error. (There may be internal protections against this in FileDescriptor, but better not to try it.)

        private static final String className = "sun.nio.ch.FileChannelImpl";
        

        Should be named something like FCHANNEL_IMPL_CLASS to reflect that it is the class name of FileChannelImpl, as well as being a constant.

                // Try this veversion first, for later JDK7 and JDK8.
        

        Typo

          private static Method openMethodV1; // The method with 5 parameters
          private static Method openMethodV2; // The method with 4 parameters
        

        Can we name these something like open5Args, open4Args?

        PassedFileChannel#open: It seems like if the JDK has a particular FileChannelImpl#open method, we should just call that method and not do any fallbacks. I don't think we will ever be in a scenario where one open method is present but returns null all the time, and we need to use the other open method.

        Show
        cmccabe Colin P. McCabe added a comment - Thanks, Kai Zheng . public void createDescriptor( String info, int length, PassedFileChannel[] fchan) This can simply return a PassedFileChannel , now that we only need to return a single thing rather than two. * @ throws IOException If the accept timed out. SocketTimeoutException is timeouts, other IOExceptions could be other things. Maybe have two lines about exceptions, one for IOE and another for timeout? recvFileChannels: you need to set descriptors [i] = null once you have created a PassedFileChannel for the descriptor. Otherwise you would try to close the same descriptor twice after an error. (There may be internal protections against this in FileDescriptor , but better not to try it.) private static final String className = "sun.nio.ch.FileChannelImpl" ; Should be named something like FCHANNEL_IMPL_CLASS to reflect that it is the class name of FileChannelImpl , as well as being a constant. // Try this veversion first, for later JDK7 and JDK8. Typo private static Method openMethodV1; // The method with 5 parameters private static Method openMethodV2; // The method with 4 parameters Can we name these something like open5Args, open4Args? PassedFileChannel#open : It seems like if the JDK has a particular FileChannelImpl#open method, we should just call that method and not do any fallbacks. I don't think we will ever be in a scenario where one open method is present but returns null all the time, and we need to use the other open method.
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 0s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.
        +1 mvninstall 8m 50s trunk passed
        +1 compile 9m 51s trunk passed with JDK v1.8.0_66
        +1 compile 10m 32s trunk passed with JDK v1.7.0_91
        +1 checkstyle 1m 10s trunk passed
        +1 mvnsite 2m 47s trunk passed
        +1 mvneclipse 0m 48s trunk passed
        +1 findbugs 6m 9s trunk passed
        +1 javadoc 2m 50s trunk passed with JDK v1.8.0_66
        +1 javadoc 3m 47s trunk passed with JDK v1.7.0_91
        +1 mvninstall 3m 32s the patch passed
        +1 compile 9m 50s the patch passed with JDK v1.8.0_66
        -1 javac 22m 7s root-jdk1.8.0_66 with JDK v1.8.0_66 generated 7 new issues (was 729, now 731).
        +1 javac 9m 50s the patch passed
        +1 compile 10m 33s the patch passed with JDK v1.7.0_91
        -1 javac 32m 40s root-jdk1.7.0_91 with JDK v1.7.0_91 generated 7 new issues (was 723, now 725).
        +1 javac 10m 33s the patch passed
        -1 checkstyle 1m 5s Patch generated 8 new checkstyle issues in root (total was 252, now 256).
        +1 mvnsite 2m 52s the patch passed
        +1 mvneclipse 0m 46s the patch passed
        +1 whitespace 0m 0s Patch has no whitespace issues.
        -1 findbugs 2m 17s hadoop-common-project/hadoop-common introduced 2 new FindBugs issues.
        +1 javadoc 2m 19s the patch passed with JDK v1.8.0_66
        +1 javadoc 3m 19s the patch passed with JDK v1.7.0_91
        -1 unit 7m 27s hadoop-common in the patch failed with JDK v1.8.0_66.
        +1 unit 0m 54s hadoop-hdfs-client in the patch passed with JDK v1.8.0_66.
        -1 unit 63m 28s hadoop-hdfs in the patch failed with JDK v1.8.0_66.
        +1 unit 7m 55s hadoop-common in the patch passed with JDK v1.7.0_91.
        +1 unit 0m 59s hadoop-hdfs-client in the patch passed with JDK v1.7.0_91.
        -1 unit 67m 43s hadoop-hdfs in the patch failed with JDK v1.7.0_91.
        +1 asflicense 0m 29s Patch does not generate ASF License warnings.
        238m 35s



        Reason Tests
        FindBugs module:hadoop-common-project/hadoop-common
          Exception is caught when Exception is not thrown in org.apache.hadoop.net.unix.PassedFileChannel.<static initializer for PassedFileChannel>() At PassedFileChannel.java:is not thrown in org.apache.hadoop.net.unix.PassedFileChannel.<static initializer for PassedFileChannel>() At PassedFileChannel.java:[line 66]
          Exception is caught when Exception is not thrown in org.apache.hadoop.net.unix.PassedFileChannel.open(FileDescriptor, boolean) At PassedFileChannel.java:is not thrown in org.apache.hadoop.net.unix.PassedFileChannel.open(FileDescriptor, boolean) At PassedFileChannel.java:[line 141]
        JDK v1.8.0_66 Failed junit tests hadoop.ipc.TestRPCWaitForProxy
          hadoop.hdfs.TestDatanodeRegistration
          hadoop.hdfs.server.datanode.TestBlockReplacement
          hadoop.hdfs.server.namenode.TestDecommissioningStatus
        JDK v1.7.0_91 Failed junit tests hadoop.hdfs.TestDFSStripedOutputStreamWithFailure200
          hadoop.hdfs.TestDatanodeDeath
          hadoop.hdfs.shortcircuit.TestShortCircuitCache



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:0ca8df7
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12779038/HDFS-8562.003b.patch
        JIRA Issue HDFS-8562
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 2dba4b767768 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 70d6f20
        findbugs v3.0.0
        javac root-jdk1.8.0_66: https://builds.apache.org/job/PreCommit-HDFS-Build/13975/artifact/patchprocess/diff-compile-javac-root-jdk1.8.0_66.txt
        javac root-jdk1.7.0_91: https://builds.apache.org/job/PreCommit-HDFS-Build/13975/artifact/patchprocess/diff-compile-javac-root-jdk1.7.0_91.txt
        checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/13975/artifact/patchprocess/diff-checkstyle-root.txt
        findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/13975/artifact/patchprocess/new-findbugs-hadoop-common-project_hadoop-common.html
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/13975/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_66.txt
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/13975/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_66.txt
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/13975/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_91.txt
        unit test logs https://builds.apache.org/job/PreCommit-HDFS-Build/13975/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_66.txt https://builds.apache.org/job/PreCommit-HDFS-Build/13975/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_66.txt https://builds.apache.org/job/PreCommit-HDFS-Build/13975/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_91.txt
        JDK v1.7.0_91 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/13975/testReport/
        modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs-client U: .
        Max memory used 76MB
        Powered by Apache Yetus 0.2.0-SNAPSHOT http://yetus.apache.org
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/13975/console

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 5 new or modified test files. +1 mvninstall 8m 50s trunk passed +1 compile 9m 51s trunk passed with JDK v1.8.0_66 +1 compile 10m 32s trunk passed with JDK v1.7.0_91 +1 checkstyle 1m 10s trunk passed +1 mvnsite 2m 47s trunk passed +1 mvneclipse 0m 48s trunk passed +1 findbugs 6m 9s trunk passed +1 javadoc 2m 50s trunk passed with JDK v1.8.0_66 +1 javadoc 3m 47s trunk passed with JDK v1.7.0_91 +1 mvninstall 3m 32s the patch passed +1 compile 9m 50s the patch passed with JDK v1.8.0_66 -1 javac 22m 7s root-jdk1.8.0_66 with JDK v1.8.0_66 generated 7 new issues (was 729, now 731). +1 javac 9m 50s the patch passed +1 compile 10m 33s the patch passed with JDK v1.7.0_91 -1 javac 32m 40s root-jdk1.7.0_91 with JDK v1.7.0_91 generated 7 new issues (was 723, now 725). +1 javac 10m 33s the patch passed -1 checkstyle 1m 5s Patch generated 8 new checkstyle issues in root (total was 252, now 256). +1 mvnsite 2m 52s the patch passed +1 mvneclipse 0m 46s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. -1 findbugs 2m 17s hadoop-common-project/hadoop-common introduced 2 new FindBugs issues. +1 javadoc 2m 19s the patch passed with JDK v1.8.0_66 +1 javadoc 3m 19s the patch passed with JDK v1.7.0_91 -1 unit 7m 27s hadoop-common in the patch failed with JDK v1.8.0_66. +1 unit 0m 54s hadoop-hdfs-client in the patch passed with JDK v1.8.0_66. -1 unit 63m 28s hadoop-hdfs in the patch failed with JDK v1.8.0_66. +1 unit 7m 55s hadoop-common in the patch passed with JDK v1.7.0_91. +1 unit 0m 59s hadoop-hdfs-client in the patch passed with JDK v1.7.0_91. -1 unit 67m 43s hadoop-hdfs in the patch failed with JDK v1.7.0_91. +1 asflicense 0m 29s Patch does not generate ASF License warnings. 238m 35s Reason Tests FindBugs module:hadoop-common-project/hadoop-common   Exception is caught when Exception is not thrown in org.apache.hadoop.net.unix.PassedFileChannel.<static initializer for PassedFileChannel>() At PassedFileChannel.java:is not thrown in org.apache.hadoop.net.unix.PassedFileChannel.<static initializer for PassedFileChannel>() At PassedFileChannel.java: [line 66]   Exception is caught when Exception is not thrown in org.apache.hadoop.net.unix.PassedFileChannel.open(FileDescriptor, boolean) At PassedFileChannel.java:is not thrown in org.apache.hadoop.net.unix.PassedFileChannel.open(FileDescriptor, boolean) At PassedFileChannel.java: [line 141] JDK v1.8.0_66 Failed junit tests hadoop.ipc.TestRPCWaitForProxy   hadoop.hdfs.TestDatanodeRegistration   hadoop.hdfs.server.datanode.TestBlockReplacement   hadoop.hdfs.server.namenode.TestDecommissioningStatus JDK v1.7.0_91 Failed junit tests hadoop.hdfs.TestDFSStripedOutputStreamWithFailure200   hadoop.hdfs.TestDatanodeDeath   hadoop.hdfs.shortcircuit.TestShortCircuitCache Subsystem Report/Notes Docker Image:yetus/hadoop:0ca8df7 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12779038/HDFS-8562.003b.patch JIRA Issue HDFS-8562 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 2dba4b767768 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 70d6f20 findbugs v3.0.0 javac root-jdk1.8.0_66: https://builds.apache.org/job/PreCommit-HDFS-Build/13975/artifact/patchprocess/diff-compile-javac-root-jdk1.8.0_66.txt javac root-jdk1.7.0_91: https://builds.apache.org/job/PreCommit-HDFS-Build/13975/artifact/patchprocess/diff-compile-javac-root-jdk1.7.0_91.txt checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/13975/artifact/patchprocess/diff-checkstyle-root.txt findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/13975/artifact/patchprocess/new-findbugs-hadoop-common-project_hadoop-common.html unit https://builds.apache.org/job/PreCommit-HDFS-Build/13975/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_66.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/13975/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_66.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/13975/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_91.txt unit test logs https://builds.apache.org/job/PreCommit-HDFS-Build/13975/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_66.txt https://builds.apache.org/job/PreCommit-HDFS-Build/13975/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_66.txt https://builds.apache.org/job/PreCommit-HDFS-Build/13975/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_91.txt JDK v1.7.0_91 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/13975/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs-client U: . Max memory used 76MB Powered by Apache Yetus 0.2.0-SNAPSHOT http://yetus.apache.org Console output https://builds.apache.org/job/PreCommit-HDFS-Build/13975/console This message was automatically generated.
        Hide
        drankye Kai Zheng added a comment -

        Note I only ran some related tests using later JDK7 and JDK8. Hope Jenkins runs but I wonder the version it uses for JDK7 version is low enough to trigger the case the method has 4 parameters. Colin would you help check the case? Looks like not easy to get an old JDK7 now. Thanks.

        Show
        drankye Kai Zheng added a comment - Note I only ran some related tests using later JDK7 and JDK8. Hope Jenkins runs but I wonder the version it uses for JDK7 version is low enough to trigger the case the method has 4 parameters. Colin would you help check the case? Looks like not easy to get an old JDK7 now. Thanks.
        Hide
        drankye Kai Zheng added a comment -

        Updated the patch as Colin suggested, mainly to use the private method by reflection.

        Show
        drankye Kai Zheng added a comment - Updated the patch as Colin suggested, mainly to use the private method by reflection.
        Hide
        drankye Kai Zheng added a comment -

        Thanks Colin for the comments and further thoughts. They sound great to me. I will update the patch.

        We should probably also make a feature request on the Oracle Java bugtracker to add a getFileDescriptor method to FileChannel

        Yanping Wang, would you comment on this? AFAIK, there is no place to fire issue request to Java in Oracle Java/OpenJDK bugtracker system unless the reporter is a member. I guess we would need to leverage some one from Oracle as according to my past experience.

        Show
        drankye Kai Zheng added a comment - Thanks Colin for the comments and further thoughts. They sound great to me. I will update the patch. We should probably also make a feature request on the Oracle Java bugtracker to add a getFileDescriptor method to FileChannel Yanping Wang , would you comment on this? AFAIK, there is no place to fire issue request to Java in Oracle Java/OpenJDK bugtracker system unless the reporter is a member. I guess we would need to leverage some one from Oracle as according to my past experience.
        Hide
        cmccabe Colin P. McCabe added a comment - - edited

        Hi Kai Zheng,

        Thanks for looking at this. 003a doesn't compile for me because I'm using Oracle Java 1.7.0_10-b18, which doesn't have the four-argument version of FileChannelImpl#open. I think we are going to need to use reflection for this to get a version that can pass Jenkins and be generally useful.

        It might be better to name FileChannelWrapper something like PassedFileChannel, to emphasize that it was and can be passed from another process.

        PassedFileChannel should keep around a FileDescriptor object, so that we don't have to modify lots and lots of functions to take two arguments instead of one.

        We should probably also make a feature request on the Oracle Java bugtracker to add a getFileDescriptor method to FileChannel. This is another example of a very useful FD-related method that FileInputStream currently has and FileChannel doesn't, for no good reason that I can see.

        Show
        cmccabe Colin P. McCabe added a comment - - edited Hi Kai Zheng , Thanks for looking at this. 003a doesn't compile for me because I'm using Oracle Java 1.7.0_10-b18, which doesn't have the four-argument version of FileChannelImpl#open . I think we are going to need to use reflection for this to get a version that can pass Jenkins and be generally useful. It might be better to name FileChannelWrapper something like PassedFileChannel , to emphasize that it was and can be passed from another process. PassedFileChannel should keep around a FileDescriptor object, so that we don't have to modify lots and lots of functions to take two arguments instead of one. We should probably also make a feature request on the Oracle Java bugtracker to add a getFileDescriptor method to FileChannel . This is another example of a very useful FD-related method that FileInputStream currently has and FileChannel doesn't, for no good reason that I can see.
        Hide
        drankye Kai Zheng added a comment -

        Put it on Jenkins and see if it works or not for all the related tests.

        Show
        drankye Kai Zheng added a comment - Put it on Jenkins and see if it works or not for all the related tests.
        Hide
        drankye Kai Zheng added a comment -

        Uploaded a version based on Colin's previous patch to address the issues as discussed above:

        • Introduced FileChannelWrapper that's equivalent to FileChannel but in fallback case it will hold the parent FileInputStream as suggested by Colin;
        • Used the FileChannelWrapper in every place that would use FileChannel instead;
          Note the life cycle of FileChannelWrapper should be the same with FileChannel. And it could be regarded as a temporary workaround for now since we don't have a public API to open a FileChannel using a FileDescriptor. When it's available in future, this wrapper class can be easily swapped out.
          Some related tests can run on both JDK7 and JDK8, but may fail on other JDK or JDK versions. In that case reflection can be used to support them but I wonder if that's really necessary since we have and need the fallback anyway.
        Show
        drankye Kai Zheng added a comment - Uploaded a version based on Colin's previous patch to address the issues as discussed above: Introduced FileChannelWrapper that's equivalent to FileChannel but in fallback case it will hold the parent FileInputStream as suggested by Colin; Used the FileChannelWrapper in every place that would use FileChannel instead; Note the life cycle of FileChannelWrapper should be the same with FileChannel. And it could be regarded as a temporary workaround for now since we don't have a public API to open a FileChannel using a FileDescriptor . When it's available in future, this wrapper class can be easily swapped out. Some related tests can run on both JDK7 and JDK8, but may fail on other JDK or JDK versions. In that case reflection can be used to support them but I wonder if that's really necessary since we have and need the fallback anyway.
        Hide
        cmccabe Colin P. McCabe added a comment -

        Thanks, Yanping Wang. Is there a bug tracker link for the issue of adding a public way of creating a FileChannel without a FileInputStream?

        Kai Zheng: I think reflection would work pretty well here. We have a lot of users on the existing JDK7 versions, so it would be nice to find something that would work there as well.

        Show
        cmccabe Colin P. McCabe added a comment - Thanks, Yanping Wang . Is there a bug tracker link for the issue of adding a public way of creating a FileChannel without a FileInputStream ? Kai Zheng : I think reflection would work pretty well here. We have a lot of users on the existing JDK7 versions, so it would be nice to find something that would work there as well.
        Hide
        drankye Kai Zheng added a comment -

        Thanks Yanping for the help and heads-up!

        Show
        drankye Kai Zheng added a comment - Thanks Yanping for the help and heads-up!
        Hide
        ywang261 Yanping Wang added a comment -

        Oracle Java team had a bug/issue filed for this issue, https://bugs.openjdk.java.net/browse/JDK-8080225 "FileInputStream cleanup should be improved."
        The solution is to use Phantom Reference instead of Final Reference. In order to make Phantom Reference works in FileInputStream and FileOutputStream properly for GC, there were two related issues: "Clear phantom reference as soft and weak references do" and "Remove REF_CLEANER reference category"

        I think your suggestion is good and reasonable, I will pass that to Oracle runtime and Openjdk teams.

        Show
        ywang261 Yanping Wang added a comment - Oracle Java team had a bug/issue filed for this issue, https://bugs.openjdk.java.net/browse/JDK-8080225 "FileInputStream cleanup should be improved." The solution is to use Phantom Reference instead of Final Reference. In order to make Phantom Reference works in FileInputStream and FileOutputStream properly for GC, there were two related issues: "Clear phantom reference as soft and weak references do" and "Remove REF_CLEANER reference category" I think your suggestion is good and reasonable, I will pass that to Oracle runtime and Openjdk teams.
        Hide
        drankye Kai Zheng added a comment -

        Hi Yanping Wang, per above discussion, the blocking issue is, we need a public Java API to create a FileChannel via a FileDescriptor without file path. Could you make a feature request to Oracle for this? I thought it's doable because the path isn't essentially needed and we do have a strong requirement here. If we're lucky, it may be done in a Java update and we won't have to wait for it in next major version. Thanks!

        Show
        drankye Kai Zheng added a comment - Hi Yanping Wang , per above discussion, the blocking issue is, we need a public Java API to create a FileChannel via a FileDescriptor without file path. Could you make a feature request to Oracle for this? I thought it's doable because the path isn't essentially needed and we do have a strong requirement here. If we're lucky, it may be done in a Java update and we won't have to wait for it in next major version. Thanks!
        Hide
        drankye Kai Zheng added a comment -

        Yeah, we may need some tweak anyway here. Can we obtain the path in addition to the fd, like passing it from DataNode if we do need it? It may be OK if we don't have it, because I guess the path parameter to call the method can be set null. Ref. the following codes from the source in OpenJDK 8:

            // The path of the referenced file
            // (null if the parent stream is created with a file descriptor)
            private final String path;
        

        Calling the method thru reflection is a good idea, the logic could be:
        1) Attempting to call FileChannelImpl.open(fd, true, true, null); // JDK 7
        2) Attempting to call FileChannelImpl.open(fd, null, true, true, null); // JDK 8, path could be nullable
        3) Fallback to FileInputStream

        Show
        drankye Kai Zheng added a comment - Yeah, we may need some tweak anyway here. Can we obtain the path in addition to the fd, like passing it from DataNode if we do need it? It may be OK if we don't have it, because I guess the path parameter to call the method can be set null. Ref. the following codes from the source in OpenJDK 8: // The path of the referenced file // ( null if the parent stream is created with a file descriptor) private final String path; Calling the method thru reflection is a good idea, the logic could be: 1) Attempting to call FileChannelImpl.open(fd, true, true, null); // JDK 7 2) Attempting to call FileChannelImpl.open(fd, null, true, true, null); // JDK 8, path could be nullable 3) Fallback to FileInputStream
        Hide
        cmccabe Colin P. McCabe added a comment -

        That's frustrating. Unfortunately, I don't think there is any file-related class in standard Java which can be created via a FileDescriptor without using private APIs-- except FileInputStream. FileChannel#open only takes paths. RandomAccessFile's constructors only take paths as well. One approach would be to try to use the private APIs via reflection, and then fall back to creating a FileInputStream if that's not possible. We can get a FileChannel from the FileInputStream. In order to do that, we'd have to hold around a reference to the FileInputStream somewhere, to prevent its finalizer from kicking in and closing the FileDescriptor.

        Show
        cmccabe Colin P. McCabe added a comment - That's frustrating. Unfortunately, I don't think there is any file-related class in standard Java which can be created via a FileDescriptor without using private APIs-- except FileInputStream . FileChannel#open only takes paths. RandomAccessFile 's constructors only take paths as well. One approach would be to try to use the private APIs via reflection, and then fall back to creating a FileInputStream if that's not possible. We can get a FileChannel from the FileInputStream . In order to do that, we'd have to hold around a reference to the FileInputStream somewhere, to prevent its finalizer from kicking in and closing the FileDescriptor .
        Hide
        drankye Kai Zheng added a comment -

        The tricky thing to call the internal method FileChannelImpl.open() is, the signature may change for different JDK versions.
        In JDK 7 ref.
        https://github.com/openjdk-mirror/jdk7u-jdk/blob/master/src/share/classes/sun/nio/ch/FileChannelImpl.java
        In JDK 8 ref.
        http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/4a1e42601d61/src/share/classes/sun/nio/ch/FileChannelImpl.java

        Show
        drankye Kai Zheng added a comment - The tricky thing to call the internal method FileChannelImpl.open() is, the signature may change for different JDK versions. In JDK 7 ref. https://github.com/openjdk-mirror/jdk7u-jdk/blob/master/src/share/classes/sun/nio/ch/FileChannelImpl.java In JDK 8 ref. http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/4a1e42601d61/src/share/classes/sun/nio/ch/FileChannelImpl.java
        Hide
        drankye Kai Zheng added a comment -

        Note the patch doesn't compile for Java 7 in the places using FileChannelImpl.open().

        Show
        drankye Kai Zheng added a comment - Note the patch doesn't compile for Java 7 in the places using FileChannelImpl.open().
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 0s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.
        +1 mvninstall 8m 17s trunk passed
        +1 compile 9m 59s trunk passed with JDK v1.8.0_66
        +1 compile 10m 16s trunk passed with JDK v1.7.0_91
        +1 checkstyle 1m 16s trunk passed
        +1 mvnsite 2m 40s trunk passed
        +1 mvneclipse 0m 42s trunk passed
        +1 findbugs 5m 59s trunk passed
        +1 javadoc 2m 28s trunk passed with JDK v1.8.0_66
        +1 javadoc 3m 28s trunk passed with JDK v1.7.0_91
        -1 mvninstall 0m 24s hadoop-common in the patch failed.
        -1 mvninstall 0m 24s hadoop-hdfs-client in the patch failed.
        -1 mvninstall 0m 30s hadoop-hdfs in the patch failed.
        -1 compile 0m 37s root in the patch failed with JDK v1.8.0_66.
        -1 javac 0m 37s root in the patch failed with JDK v1.8.0_66.
        -1 compile 0m 39s root in the patch failed with JDK v1.7.0_91.
        -1 javac 0m 39s root in the patch failed with JDK v1.7.0_91.
        -1 checkstyle 1m 2s Patch generated 18 new checkstyle issues in root (total was 251, now 266).
        -1 mvnsite 0m 26s hadoop-common in the patch failed.
        -1 mvnsite 0m 26s hadoop-hdfs-client in the patch failed.
        -1 mvnsite 0m 31s hadoop-hdfs in the patch failed.
        +1 mvneclipse 0m 40s the patch passed
        +1 whitespace 0m 0s Patch has no whitespace issues.
        -1 findbugs 0m 23s hadoop-common in the patch failed.
        -1 findbugs 0m 24s hadoop-hdfs-client in the patch failed.
        -1 findbugs 0m 29s hadoop-hdfs in the patch failed.
        +1 javadoc 2m 31s the patch passed with JDK v1.8.0_66
        -1 javadoc 7m 52s hadoop-common-project_hadoop-common-jdk1.7.0_91 with JDK v1.7.0_91 generated 1 new issues (was 13, now 14).
        -1 javadoc 7m 52s hadoop-hdfs-project_hadoop-hdfs-client-jdk1.7.0_91 with JDK v1.7.0_91 generated 1 new issues (was 1, now 2).
        +1 javadoc 3m 26s the patch passed with JDK v1.7.0_91
        -1 unit 0m 25s hadoop-common in the patch failed with JDK v1.8.0_66.
        -1 unit 0m 33s hadoop-hdfs-client in the patch failed with JDK v1.8.0_66.
        -1 unit 0m 36s hadoop-hdfs in the patch failed with JDK v1.8.0_66.
        -1 unit 0m 27s hadoop-common in the patch failed with JDK v1.7.0_91.
        -1 unit 0m 29s hadoop-hdfs-client in the patch failed with JDK v1.7.0_91.
        -1 unit 0m 35s hadoop-hdfs in the patch failed with JDK v1.7.0_91.
        +1 asflicense 0m 25s Patch does not generate ASF License warnings.
        62m 40s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:0ca8df7
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12777615/HDFS-8562.002b.patch
        JIRA Issue HDFS-8562
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux be73d4ad9745 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / d8a4542
        findbugs v3.0.0
        mvninstall https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-mvninstall-hadoop-common-project_hadoop-common.txt
        mvninstall https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-mvninstall-hadoop-hdfs-project_hadoop-hdfs-client.txt
        mvninstall https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-mvninstall-hadoop-hdfs-project_hadoop-hdfs.txt
        compile https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-compile-root-jdk1.8.0_66.txt
        javac https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-compile-root-jdk1.8.0_66.txt
        compile https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-compile-root-jdk1.7.0_91.txt
        javac https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-compile-root-jdk1.7.0_91.txt
        checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/diff-checkstyle-root.txt
        mvnsite https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-mvnsite-hadoop-common-project_hadoop-common.txt
        mvnsite https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-mvnsite-hadoop-hdfs-project_hadoop-hdfs-client.txt
        mvnsite https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-mvnsite-hadoop-hdfs-project_hadoop-hdfs.txt
        findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-findbugs-hadoop-common-project_hadoop-common.txt
        findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-findbugs-hadoop-hdfs-project_hadoop-hdfs-client.txt
        findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-findbugs-hadoop-hdfs-project_hadoop-hdfs.txt
        javadoc hadoop-common-project_hadoop-common-jdk1.7.0_91: https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/diff-javadoc-javadoc-hadoop-common-project_hadoop-common-jdk1.7.0_91.txt
        javadoc hadoop-hdfs-project_hadoop-hdfs-client-jdk1.7.0_91: https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/diff-javadoc-javadoc-hadoop-hdfs-project_hadoop-hdfs-client-jdk1.7.0_91.txt
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_66.txt
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-client-jdk1.8.0_66.txt
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_66.txt
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_91.txt
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-client-jdk1.7.0_91.txt
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_91.txt
        JDK v1.7.0_91 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/13873/testReport/
        modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs-client U: .
        Max memory used 76MB
        Powered by Apache Yetus 0.1.0 http://yetus.apache.org
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/13873/console

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 5 new or modified test files. +1 mvninstall 8m 17s trunk passed +1 compile 9m 59s trunk passed with JDK v1.8.0_66 +1 compile 10m 16s trunk passed with JDK v1.7.0_91 +1 checkstyle 1m 16s trunk passed +1 mvnsite 2m 40s trunk passed +1 mvneclipse 0m 42s trunk passed +1 findbugs 5m 59s trunk passed +1 javadoc 2m 28s trunk passed with JDK v1.8.0_66 +1 javadoc 3m 28s trunk passed with JDK v1.7.0_91 -1 mvninstall 0m 24s hadoop-common in the patch failed. -1 mvninstall 0m 24s hadoop-hdfs-client in the patch failed. -1 mvninstall 0m 30s hadoop-hdfs in the patch failed. -1 compile 0m 37s root in the patch failed with JDK v1.8.0_66. -1 javac 0m 37s root in the patch failed with JDK v1.8.0_66. -1 compile 0m 39s root in the patch failed with JDK v1.7.0_91. -1 javac 0m 39s root in the patch failed with JDK v1.7.0_91. -1 checkstyle 1m 2s Patch generated 18 new checkstyle issues in root (total was 251, now 266). -1 mvnsite 0m 26s hadoop-common in the patch failed. -1 mvnsite 0m 26s hadoop-hdfs-client in the patch failed. -1 mvnsite 0m 31s hadoop-hdfs in the patch failed. +1 mvneclipse 0m 40s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. -1 findbugs 0m 23s hadoop-common in the patch failed. -1 findbugs 0m 24s hadoop-hdfs-client in the patch failed. -1 findbugs 0m 29s hadoop-hdfs in the patch failed. +1 javadoc 2m 31s the patch passed with JDK v1.8.0_66 -1 javadoc 7m 52s hadoop-common-project_hadoop-common-jdk1.7.0_91 with JDK v1.7.0_91 generated 1 new issues (was 13, now 14). -1 javadoc 7m 52s hadoop-hdfs-project_hadoop-hdfs-client-jdk1.7.0_91 with JDK v1.7.0_91 generated 1 new issues (was 1, now 2). +1 javadoc 3m 26s the patch passed with JDK v1.7.0_91 -1 unit 0m 25s hadoop-common in the patch failed with JDK v1.8.0_66. -1 unit 0m 33s hadoop-hdfs-client in the patch failed with JDK v1.8.0_66. -1 unit 0m 36s hadoop-hdfs in the patch failed with JDK v1.8.0_66. -1 unit 0m 27s hadoop-common in the patch failed with JDK v1.7.0_91. -1 unit 0m 29s hadoop-hdfs-client in the patch failed with JDK v1.7.0_91. -1 unit 0m 35s hadoop-hdfs in the patch failed with JDK v1.7.0_91. +1 asflicense 0m 25s Patch does not generate ASF License warnings. 62m 40s Subsystem Report/Notes Docker Image:yetus/hadoop:0ca8df7 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12777615/HDFS-8562.002b.patch JIRA Issue HDFS-8562 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux be73d4ad9745 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / d8a4542 findbugs v3.0.0 mvninstall https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-mvninstall-hadoop-common-project_hadoop-common.txt mvninstall https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-mvninstall-hadoop-hdfs-project_hadoop-hdfs-client.txt mvninstall https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-mvninstall-hadoop-hdfs-project_hadoop-hdfs.txt compile https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-compile-root-jdk1.8.0_66.txt javac https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-compile-root-jdk1.8.0_66.txt compile https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-compile-root-jdk1.7.0_91.txt javac https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-compile-root-jdk1.7.0_91.txt checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/diff-checkstyle-root.txt mvnsite https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-mvnsite-hadoop-common-project_hadoop-common.txt mvnsite https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-mvnsite-hadoop-hdfs-project_hadoop-hdfs-client.txt mvnsite https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-mvnsite-hadoop-hdfs-project_hadoop-hdfs.txt findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-findbugs-hadoop-common-project_hadoop-common.txt findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-findbugs-hadoop-hdfs-project_hadoop-hdfs-client.txt findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-findbugs-hadoop-hdfs-project_hadoop-hdfs.txt javadoc hadoop-common-project_hadoop-common-jdk1.7.0_91: https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/diff-javadoc-javadoc-hadoop-common-project_hadoop-common-jdk1.7.0_91.txt javadoc hadoop-hdfs-project_hadoop-hdfs-client-jdk1.7.0_91: https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/diff-javadoc-javadoc-hadoop-hdfs-project_hadoop-hdfs-client-jdk1.7.0_91.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_66.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-client-jdk1.8.0_66.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.8.0_66.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_91.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-client-jdk1.7.0_91.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/13873/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_91.txt JDK v1.7.0_91 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/13873/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs-client U: . Max memory used 76MB Powered by Apache Yetus 0.1.0 http://yetus.apache.org Console output https://builds.apache.org/job/PreCommit-HDFS-Build/13873/console This message was automatically generated.
        Hide
        drankye Kai Zheng added a comment -

        Thanks Colin for providing the formal patch. It looks a good rational to just focus on the key path and limit the scope. Please take it accordingly if you'd like to. Thanks.

        Show
        drankye Kai Zheng added a comment - Thanks Colin for providing the formal patch. It looks a good rational to just focus on the key path and limit the scope. Please take it accordingly if you'd like to. Thanks.
        Hide
        cmccabe Colin P. McCabe added a comment -

        HBase keeps open many HDFS files. Because of the way short-circuit reads work, this involves keeping a large cache of FileInputStream pairs-- sometimes as many as 128,000. I think it's those finalizers that are the problem here. Those finalizers are located in the HDFS client, not in the DataNode. In contrast to the HDFS client, the DataNode does not keep files open for very long (for better or for worse)... when it's done reading a block file, it immediately closes it. So the DN is not likely to have more than a dozen or two FileInputStream objects around at once. I do not think that that will have a major impact on GC. DN heaps are typically small anyway, and almost never have GC problems.

        I think the best thing to do would be to move towards using FileChannel in the HDFS client. I have a patch to do this which I'll post shortly.

        Show
        cmccabe Colin P. McCabe added a comment - HBase keeps open many HDFS files. Because of the way short-circuit reads work, this involves keeping a large cache of FileInputStream pairs-- sometimes as many as 128,000. I think it's those finalizers that are the problem here. Those finalizers are located in the HDFS client, not in the DataNode. In contrast to the HDFS client, the DataNode does not keep files open for very long (for better or for worse)... when it's done reading a block file, it immediately closes it. So the DN is not likely to have more than a dozen or two FileInputStream objects around at once. I do not think that that will have a major impact on GC. DN heaps are typically small anyway, and almost never have GC problems. I think the best thing to do would be to move towards using FileChannel in the HDFS client. I have a patch to do this which I'll post shortly.
        Hide
        drankye Kai Zheng added a comment -

        Per off-line discussion with Wei, I assigned this to him. Thanks for the taking!

        Show
        drankye Kai Zheng added a comment - Per off-line discussion with Wei, I assigned this to him. Thanks for the taking!
        Hide
        drankye Kai Zheng added a comment -

        Thanks Haohui Mai for the confirm. I thought we're clear about the direction.

        Hi Wei Zhou, I looked at the initial patch, and it looks like in the good way. Some comments so far:
        1. I understand there're some cases that we need an InputStream anyway because we want to avoid change the interface, so the new class AltFileInputStream is given as an alternative class for FileInputStream. Could we give a better comment for it in the class header? And please remove those unnecessary comments inherited from InputStream.

        2. As both Colin and Haohui suggested, what we need is to use FileChannel instead of FileInputStream or avoid InputStream at all. So for the places that we can avoid using whatever an InputStream, we might also want to avoid using the new AltFileInputStream, instead try to use FileChannel directly.

        3. The patch seems to cover NameNode, DataNode and client. If that's the case, we may miss some places, like fsimage/editlog loading cases. If the end resultant patch is big and hard for review, maybe we can break it down into separate issues.

        4. When the patch is in good shape, I thought we can submit it for formal review.

        Thanks.

        Show
        drankye Kai Zheng added a comment - Thanks Haohui Mai for the confirm. I thought we're clear about the direction. Hi Wei Zhou , I looked at the initial patch, and it looks like in the good way. Some comments so far: 1. I understand there're some cases that we need an InputStream anyway because we want to avoid change the interface, so the new class AltFileInputStream is given as an alternative class for FileInputStream . Could we give a better comment for it in the class header? And please remove those unnecessary comments inherited from InputStream. 2. As both Colin and Haohui suggested, what we need is to use FileChannel instead of FileInputStream or avoid InputStream at all. So for the places that we can avoid using whatever an InputStream, we might also want to avoid using the new AltFileInputStream, instead try to use FileChannel directly. 3. The patch seems to cover NameNode, DataNode and client. If that's the case, we may miss some places, like fsimage/editlog loading cases. If the end resultant patch is big and hard for review, maybe we can break it down into separate issues. 4. When the patch is in good shape, I thought we can submit it for formal review. Thanks.
        Hide
        wheat9 Haohui Mai added a comment -

        I agree with Colin that we should move away from FileInputStream and migrate towards FileChannel.

        Show
        wheat9 Haohui Mai added a comment - I agree with Colin that we should move away from FileInputStream and migrate towards FileChannel.
        Hide
        zhouwei Wei Zhou added a comment -

        This is the patch mentioned in Kai's comment. This patch is for POC and evaluation. Thanks!

        Show
        zhouwei Wei Zhou added a comment - This is the patch mentioned in Kai's comment. This patch is for POC and evaluation. Thanks!
        Hide
        drankye Kai Zheng added a comment -

        Thanks Colin P. McCabe for the interests. We have a rough prototype codes in the way as you thought, and because we don't have the chance to verify the effect, we don't upload it yet. Maybe we could attach it for you to take a look? Thanks!

        Show
        drankye Kai Zheng added a comment - Thanks Colin P. McCabe for the interests. We have a rough prototype codes in the way as you thought, and because we don't have the chance to verify the effect, we don't upload it yet. Maybe we could attach it for you to take a look? Thanks!
        Hide
        cmccabe Colin P. McCabe added a comment -

        Thanks for reporting this. I think it would be a major maintenance burden to duplicate all the code in DFSInputStream. Instead, we should just use FileChannel instead of FileInputStream.

        Show
        cmccabe Colin P. McCabe added a comment - Thanks for reporting this. I think it would be a major maintenance burden to duplicate all the code in DFSInputStream. Instead, we should just use FileChannel instead of FileInputStream .
        Hide
        minglei mingleizhang added a comment -

        Thank you,Yanping.I'm trying to solve these problems these days.

        Show
        minglei mingleizhang added a comment - Thank you,Yanping.I'm trying to solve these problems these days.
        Hide
        ywang261 Yanping Wang added a comment -

        Comment from Charlie Hunt (Oracle Java Performance):

        We have been discussing the issue here too. As you mentioned in the JIRA report, it may take a while to get addressed in FileInputStream. We also realized that FIleInputStream’s finalize() method is documented in the APIs for FileInputStream. That implies with a change to remove the finalizer requires a specification change for FileInputStream. Changing the spec may require us to remove the finalizer in JDK 9 and not be able to back port to a JDK 8 update release. The thought is that we shouldn’t change API specifications in an update release, and I think that sounds reasonable since there could be someone who has extended FileInputStream and overridden a FileInputStream’s finalize() method to do some additional cleanup work, and also call FileInputStream’s finalize() method, (note, that FileInputStream.finalize() is a protected method which means classes that extend FileInputStream can call it). For such an application that might do that sort of thing, if we remove the finalize() method, we would break that application.

        We would like HDFS community to help us to understand how the FileInputStream is being used. Where are the code in HDFS that calls the constructor that has the FileInputStream as an arg?

        Show
        ywang261 Yanping Wang added a comment - Comment from Charlie Hunt (Oracle Java Performance): We have been discussing the issue here too. As you mentioned in the JIRA report, it may take a while to get addressed in FileInputStream. We also realized that FIleInputStream’s finalize() method is documented in the APIs for FileInputStream. That implies with a change to remove the finalizer requires a specification change for FileInputStream. Changing the spec may require us to remove the finalizer in JDK 9 and not be able to back port to a JDK 8 update release. The thought is that we shouldn’t change API specifications in an update release, and I think that sounds reasonable since there could be someone who has extended FileInputStream and overridden a FileInputStream’s finalize() method to do some additional cleanup work, and also call FileInputStream’s finalize() method, (note, that FileInputStream.finalize() is a protected method which means classes that extend FileInputStream can call it). For such an application that might do that sort of thing, if we remove the finalize() method, we would break that application. We would like HDFS community to help us to understand how the FileInputStream is being used. Where are the code in HDFS that calls the constructor that has the FileInputStream as an arg?
        Hide
        ywang261 Yanping Wang added a comment -

        Work remotely from 6/5 to 6/16, OOP 6/17-19. Expect some delay response. Thanks

        Show
        ywang261 Yanping Wang added a comment - Work remotely from 6/5 to 6/16, OOP 6/17-19. Expect some delay response. Thanks

          People

          • Assignee:
            Unassigned
            Reporter:
            ywang261 Yanping Wang
          • Votes:
            2 Vote for this issue
            Watchers:
            34 Start watching this issue

            Dates

            • Due:
              Created:
              Updated:

              Development